Add notification on change ui #1220

PaulRosset · 2017-12-01T19:18:09Z

Hi :)

Add new features on Preferences settings, concerning the Ui Tab and Hotkey Tab:

When the configuration settings is changing but you didn't saved yet, it tell you that is not saved yet.

…cation-onChangeUi

BoostnoteBot · 2017-12-01T19:18:10Z

Please make sure to be pasted screenshots of all your changes.

sota1235 · 2017-12-02T07:43:13Z

browser/main/modals/PreferencesModal/HotkeyTab.js

    }
+    if (JSON.parse(localStorage.getItem('config'))) {
+      const {hotkey} = JSON.parse(localStorage.getItem('config'))
+      this.hotkey = hotkey


You can get hotkey from this.state.config

sota1235 · 2017-12-02T07:43:41Z

browser/main/modals/PreferencesModal/HotkeyTab.js

      config: newConfig
    })
    this.clearMessage()
+    this.props.haveToSave(null)


You don't need to pass null

sota1235 · 2017-12-02T07:45:54Z

browser/main/modals/PreferencesModal/PreferencesModal.styl

+  .saving--warning
+    color #FFA500
+    font-size 10px
+    margin-top 3px


Please make this style as mixin
It seems that styles are same

sota1235 · 2017-12-02T07:46:30Z

browser/main/modals/PreferencesModal/UiTab.js

-
-    this.setState({ config: newConfig, codemirrorTheme: newCodemirrorTheme })
+    this.setState({ config: newConfig, codemirrorTheme: newCodemirrorTheme }, () => {
+      if (JSON.parse(localStorage.getItem('config'))) {


Please get config from this.props.config

sota1235 · 2017-12-02T07:47:17Z

browser/main/modals/PreferencesModal/UiTab.js

      config: props.config,
-      codemirrorTheme: props.config.editor.theme
+      codemirrorTheme: props.config.editor.theme,
+      haveToSave: null


You can remove this line (this.state.haveToSave is not used)

sota1235 · 2017-12-02T07:48:36Z

browser/main/modals/PreferencesModal/index.js

-      currentTab: 'STORAGES'
+      currentTab: 'STORAGES',
+      UI: null,
+      Hotkey: null


Hotkey is a little bit ambiguous.
For example, how about HotkeyMessage?

And I think setting default value '' is better than null

sota1235 · 2017-12-02T07:50:29Z

browser/main/modals/PreferencesModal/index.js

          <span styleName='nav-button-label'>
            {tab.label}
          </span>
+          {isOk && tab.label === tab[tab.label].tab ? <p styleName={`saving--${tab[tab.label].type}`}>{tab[tab.label].message}</p> : null}


It is little difficult to read.
Could you add class method to get Component?

sota1235 · 2017-12-02T07:54:00Z

browser/main/modals/PreferencesModal/index.js


    const navButtons = tabs.map((tab) => {
      const isActive = this.state.currentTab === tab.target
+      const isOk = typeof tab[tab.label] !== 'undefined' && tab[tab.label] !== null


isOk is a little bit simple word. Plz specify more detail for variable name like isOkToSave or anything

kazup01 · 2017-12-05T08:34:09Z

Hi @PaulRosset , could you check the comment from sota1235?

PaulRosset · 2017-12-05T08:36:27Z

Hi :) Yes, I've already check, I will try as soon as possible to push the new version, pretty busy week :)

kazup01 · 2017-12-05T08:40:51Z

@PaulRosset That's ok, thanks for your support everytime :)

PaulRosset · 2017-12-05T18:33:19Z

Hi again @sota1235 !

Made some changes to make it cleaner, tell me if it's ok for you :)

sota1235 · 2017-12-13T15:40:52Z

browser/main/modals/PreferencesModal/HotkeyTab.js

    this.setState({
      config
    })
+    if (JSON.stringify(this.oldHotkey) === JSON.stringify(config.hotkey)) {


Couldn't you write this.oldHotkey === config.hotkey?

No, I can't, because object are passed by reference, then the "===" operator is checking if both variables got the same address, since not, it can't work.

sota1235 · 2017-12-13T15:42:56Z

browser/main/modals/PreferencesModal/UiTab.js

+    this.setState({ config: newConfig, codemirrorTheme: newCodemirrorTheme }, () => {
+      const {ui, editor, preview} = this.props.config
+      this.currentConfig = {ui, editor, preview}
+      if (JSON.stringify(this.currentConfig) === JSON.stringify(this.state.config)) {


Couldn't you write this logic without JSON.stringify? It will be overhead.
If I try to write this logic, I will use _.isEqual.
https://lodash.com/docs/4.17.4#isEqual

Yes, I can use isEqual from lodash, it is a more robust solution than JSON.stringify.

sota1235 · 2017-12-13T15:44:13Z

browser/main/modals/PreferencesModal/index.js


+  haveToSaveNotif (tab) {
+    return (
+      <p styleName={`saving--${tab[tab.label].type}`}>{tab[tab.label].message}</p>


It seems that this function only need two variable.
Could you write like this?

haveToSaveNotif (type, message) { return ( // ) }

It will be easier to maintain.

sota1235

plz confirm my comments

kazup01 · 2017-12-18T08:13:37Z

@sota1235 Hey Sota-san, please review it again.

sota1235

thanks for fixing

kazup01

Works fine

kazup01 · 2017-12-28T01:45:35Z

Merged. Thanks for your contribution @PaulRosset 🎉

kazup01 · 2018-01-13T19:59:28Z

Hi @PaulRosset , thanks for your support! We were released v0.8.20.
Release Note
Enjoy Boostnote :)

PaulRosset added 3 commits December 1, 2017 15:23

Add notification when not saved

b4a7b54

Merge branch 'master' of github.com:BoostIO/Boostnote into add-notifi…

c86e451

…cation-onChangeUi

Correction UiTab

d1e5781

kazup01 requested a review from sota1235 December 2, 2017 02:20

kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Dec 2, 2017

sota1235 reviewed Dec 2, 2017

View reviewed changes

kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 2, 2017

PaulRosset added 4 commits December 5, 2017 18:16

Requested Review

f4fd131

Correction eslint code format

83910b5

Change name for state

125a493

refactor

cb59458

kazup01 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Dec 6, 2017

kazup01 assigned sota1235 Dec 7, 2017

sota1235 reviewed Dec 13, 2017

View reviewed changes

sota1235 removed their assignment Dec 13, 2017

sota1235 suggested changes Dec 13, 2017

View reviewed changes

kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 14, 2017

Add isEqual and changing haveToSaveNotif method

795fe8a

kazup01 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Dec 18, 2017

sota1235 approved these changes Dec 27, 2017

View reviewed changes

sota1235 added next release (v0.8.20) and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 27, 2017

kazup01 self-requested a review December 28, 2017 01:45

kazup01 approved these changes Dec 28, 2017

View reviewed changes

kazup01 merged commit c8cf353 into BoostIO:master Dec 28, 2017

BoostnoteBot added the Next Release label Dec 28, 2017

kazup01 removed the Next Release label Dec 28, 2017

Rokt33r mentioned this pull request Jan 13, 2018

v0.8.20 #1395

Merged

kazup01 removed the next release (v0.8.20) label Jan 13, 2018

Uh oh!

Add notification on change ui #1220

Add notification on change ui #1220

Conversation

PaulRosset commented Dec 1, 2017

Uh oh!

BoostnoteBot commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sota1235 Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazup01 commented Dec 5, 2017

Uh oh!

PaulRosset commented Dec 5, 2017

Uh oh!

kazup01 commented Dec 5, 2017

Uh oh!

PaulRosset commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sota1235 left a comment

Choose a reason for hiding this comment

Uh oh!

kazup01 commented Dec 18, 2017

Uh oh!

sota1235 left a comment

Choose a reason for hiding this comment

Uh oh!

kazup01 left a comment

Choose a reason for hiding this comment

Uh oh!

kazup01 commented Dec 28, 2017

Uh oh!

kazup01 commented Jan 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sota1235 Dec 2, 2017 •

edited

Loading