Validate bookmark name before creation Fixes #3188 #3190
Conversation
|
Reviewers: @bbondy @bridiver @bsclifton |
| @@ -12,7 +11,7 @@ const KeyCodes = require('../constants/keyCodes') | |||
| const siteTags = require('../constants/siteTags') | |||
| const siteUtil = require('../state/siteUtil') | |||
|
|
|||
| class AddEditBookmark extends ImmutableComponent { | |||
| class AddEditBookmark extends React.Component { | |||
There was a problem hiding this comment.
Thanks for the patch.
Please keep this as ImmutableComponent, and then just determine in render if it's valid or not and render accordingly. Also I think this only styles that it's invalid currently and not stops it from submitting it.
For any visual changes also pls include a screenshot, thanks.
|
@bbondy Styles are updated and the class as well. The only visual change that I introduce and has to be re rendered is when the state changes to invalid: Of course it stops the bookmark folder from saving, take a look into the Let me know if I have to tweak something else if my approach into this is not correct. |
|
Sorry it doesn't look disabled to me, @bradleyrichter pls provide styling suggestions for the save button. The red around the field is good though. |
|
@bbondy Oh well I am only applying css style to the input, not on the button. What would you say to gray it out like this, until name is empty? PS. Updated the PR and fixed an error that was present in |
|
@Sh1d0w thanks for working on this. I would prefer to only use the full-browser grey-out for full-modal dialogs. (needs to be spec'd) I think your greyed button above combined with the red focus indication will be fine. This dialog should also have a cancel button so you can give up and start over/leave with no changes. Later we can style it more like the new BM door hanger: #2894 |
|
@bradleyrichter Thanks. No problems, I like the Brave's idea and I am happy to contribute with what I can. I just didn't get should I implement the cancel button and should I make any further changes in order the PR to be accepted or it is good to go like this? It is just for fixing the currenct functionality, not to introduce new one at first place. The designs look very cool, btw! |
|
I'll add a few more follow up changes, we really appreciate the PR! |
| this.state = { | ||
| bookmarkNameValid: true, | ||
| saveButtonDisabled: true | ||
| }; |
There was a problem hiding this comment.
I'd recommend that you have an accessor defined for this instead:
get bookmarkNameValid () {
...
}
Note that there is an onChange handler on the bookmark name so the site data is always known, so you can just check it there in this.props.currentDetail.
I think you don't need to know whether the bookmark name is valid separately from if the save button should be disabled. In particular the save button should be disabled when the bookmark name is invalid.
There was a problem hiding this comment.
The only reason I did this is because if I use only one porperty to check for both the button and the input, is that the input will be highlighted in red intially, which I think should happen only if you submit empty string. However with the disabling of the button if it is empty it is not necessary, so I will refactor.
Thanks for the feedback.
|
OK submitted the review feedback, pls also shoot me an email at brian@brave.com and we'll get you on the community slack channel if you want. |
|
@bbondy Thanks for the great feedback. I have refactored the code according to your advices. |
|
super clean, thanks! |
| get isBlankTab () { | ||
| return ['about:blank', 'about:newtab'].includes(this.props.currentDetail.get('location')) | ||
| } | ||
|
|
||
| get bookmarkNameValid () { | ||
| return (this.props.currentDetail.get('title') || this.props.currentDetail.get('customTitle')) |
There was a problem hiding this comment.
Actually I'll ask for a small follow up if you don't mind, I think no title is valid for bookmarks but not bookmark folders. In that case we display as just the URL.
So the follow up PR would just have something like return !this.isFolder || ...
There was a problem hiding this comment.
Actually in case of Bookmark this.props.currentDetail.get('title') will always be set as the website's title, so even if you have empty customTItle, you should not see grayed button and if you submit the title will fallback to website's title.
git rebase -ito squash commits if needed.