-
-
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
Componentnize NewNoteButton #800
Componentnize NewNoteButton #800
Conversation
Please make sure to be pasted screenshots of all your changes. |
|
||
// Find first storage | ||
if (storage == null) { | ||
for (let kv of data.storageMap) { |
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.
I want to seek better way: for
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.
I know this if
condition is not good, but I don't know what exactly it is. So I choose a way not to change them.
browser/main/NewNoteButton/index.js
Outdated
break | ||
} | ||
} | ||
if (storage == null) window.alert('No storage to create a note') |
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.
I want to seek better way: window.alert
browser/main/NewNoteButton/index.js
Outdated
|
||
createNote (noteType) { | ||
let { dispatch, location } = this.props | ||
if (noteType !== 'MARKDOWN_NOTE' && noteType !== 'SNIPPET_NOTE') throw new Error('Invalid note type.') |
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.
Is this really needed?
browser/main/NewNoteButton/index.js
Outdated
|
||
let { storage, folder } = this.resolveTargetFolder() | ||
|
||
let newNote = noteType === 'MARKDOWN_NOTE' |
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.
I want to seek better way
browser/main/NewNoteButton/index.js
Outdated
}) | ||
} | ||
|
||
setDefaultNote (defaultNote) { |
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.
Is this really used?
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 was used in the old version, but not used now.
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.
Understood.
height $topBar-height - 1 | ||
margin-left: auto; | ||
width: 64px; | ||
margin-right: -15px; |
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.
I added these three lines for this change.
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!
return | ||
} | ||
|
||
switch (config.ui.defaultNote) { |
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.
Deleted because defaultNote
is not configurable in Preferences now.
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!
@@ -147,60 +66,6 @@ class TopBar extends React.Component { | |||
} | |||
} | |||
|
|||
createNote (noteType) { |
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.
Deleted accompanied with handleNewPostButtonClick()
}) | ||
} | ||
|
||
setDefaultNote (defaultNote) { |
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.
Deleted because it's no longer used.
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.
👍
@@ -1,51 +0,0 @@ | |||
import React, { PropTypes } from 'react' |
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.
Deleted because this file is no longer used.
browser/main/NewNoteButton/index.js
Outdated
|
||
render () { | ||
const { config, style, data, location } = this.props | ||
if (location.pathname === '/trashed') { |
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.
I changed a specification.
before
NewNoteButton was shown in Trash
but a warning appeared when a user clicked this.
after
NewNoteButton is no longer shown in Trash
.
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.
Great🎉
context
Separation
NewNoteButton
fromTopBar
.before
before(Trash)
after
I hope the layout isn't changed.
after(Trash)
for tests
note
I just move the code, so I'll have to refactor them.
memo: delete DeleteArticleModal
Make sure whether all props are passed properly.