-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add notification on change ui #1220
Add notification on change ui #1220
Conversation
Please make sure to be pasted screenshots of all your changes. |
@@ -32,6 +32,10 @@ class HotkeyTab extends React.Component { | |||
message: err.message != null ? err.message : 'Error occurs!' | |||
}}) | |||
} | |||
if (JSON.parse(localStorage.getItem('config'))) { | |||
const {hotkey} = JSON.parse(localStorage.getItem('config')) | |||
this.hotkey = hotkey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get hotkey
from this.state.config
@@ -53,6 +57,7 @@ class HotkeyTab extends React.Component { | |||
config: newConfig | |||
}) | |||
this.clearMessage() | |||
this.props.haveToSave(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to pass null
.saving--warning | ||
color #FFA500 | ||
font-size 10px | ||
margin-top 3px |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this style as mixin
It seems that styles are same
|
||
this.setState({ config: newConfig, codemirrorTheme: newCodemirrorTheme }) | ||
this.setState({ config: newConfig, codemirrorTheme: newCodemirrorTheme }, () => { | ||
if (JSON.parse(localStorage.getItem('config'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please get config from this.props.config
@@ -20,7 +20,8 @@ class UiTab extends React.Component { | |||
super(props) | |||
this.state = { | |||
config: props.config, | |||
codemirrorTheme: props.config.editor.theme | |||
codemirrorTheme: props.config.editor.theme, | |||
haveToSave: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this line (this.state.haveToSave
is not used)
currentTab: 'STORAGES' | ||
currentTab: 'STORAGES', | ||
UI: null, | ||
Hotkey: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hotkey
is a little bit ambiguous.
For example, how about HotkeyMessage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think setting default value ''
is better than null
@@ -118,6 +123,7 @@ class Preferences extends React.Component { | |||
<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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is little difficult to read.
Could you add class method to get Component?
{target: 'INFO', label: 'Community / Info'}, | ||
{target: 'CROWDFUNDING', label: 'Crowdfunding'} | ||
] | ||
|
||
const navButtons = tabs.map((tab) => { | ||
const isActive = this.state.currentTab === tab.target | ||
const isOk = typeof tab[tab.label] !== 'undefined' && tab[tab.label] !== null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOk
is a little bit simple word. Plz specify more detail for variable name like isOkToSave
or anything
Hi @PaulRosset , could you check the comment from sota1235? |
Hi :) Yes, I've already check, I will try as soon as possible to push the new version, pretty busy week :) |
@PaulRosset That's ok, thanks for your support everytime :) |
Hi again @sota1235 ! Made some changes to make it cleaner, tell me if it's ok for you :) |
@@ -70,6 +72,15 @@ class HotkeyTab extends React.Component { | |||
this.setState({ | |||
config | |||
}) | |||
if (JSON.stringify(this.oldHotkey) === JSON.stringify(config.hotkey)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you write this.oldHotkey === config.hotkey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can use isEqual
from lodash, it is a more robust solution than JSON.stringify
.
@@ -94,19 +98,26 @@ class Preferences extends React.Component { | |||
return node.getBoundingClientRect() | |||
} | |||
|
|||
haveToSaveNotif (tab) { | |||
return ( | |||
<p styleName={`saving--${tab[tab.label].type}`}>{tab[tab.label].message}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this function only need two variable.
Could you write like this?
haveToSaveNotif (type, message) {
return (
//
)
}
It will be easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz confirm my comments
@sota1235 Hey Sota-san, please review it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine
Merged. Thanks for your contribution @PaulRosset 🎉 |
Hi @PaulRosset , thanks for your support! We were released v0.8.20. |
Hi :)
When the configuration settings is changing but you didn't saved yet, it tell you that is not saved yet.