Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Validate bookmark name before creation Fixes #3188 #3190

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Validate bookmark name before creation Fixes #3188 #3190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions js/components/addEditBookmark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ class AddEditBookmark extends ImmutableComponent {
this.onSave = this.onSave.bind(this)
this.onRemoveBookmark = this.onRemoveBookmark.bind(this)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And remove this shouldComponentUpdate, no immutable component should need to ever override it.


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'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 || ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok great then

}

get isFolder () {
return siteUtil.isFolder(this.props.currentDetail)
}
Expand Down Expand Up @@ -68,6 +74,7 @@ class AddEditBookmark extends ImmutableComponent {
} else {
currentDetail = currentDetail.set('customTitle', e.target.value)
}

windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail)
}
onLocationChange (e) {
Expand All @@ -79,6 +86,11 @@ class AddEditBookmark extends ImmutableComponent {
windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail)
}
onSave () {
// First check if the title of the currentDetail is set
if (!this.bookmarkNameValid) {
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you'd check
if (!this.bookmarkNameValid) {
return false
}


const tag = this.isFolder ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK
appActions.addSite(this.props.currentDetail, tag, this.props.originalDetail, this.props.destinationDetail)
this.onClose()
Expand All @@ -91,7 +103,7 @@ class AddEditBookmark extends ImmutableComponent {
if (this.props.currentDetail.get('customTitle') !== undefined) {
return this.props.currentDetail.get('customTitle')
}
return this.props.currentDetail.get('title')
return this.props.currentDetail.get('title') || ''
}
render () {
return <Dialog onHide={this.onClose} isClickDismiss>
Expand Down Expand Up @@ -125,7 +137,7 @@ class AddEditBookmark extends ImmutableComponent {
? <a data-l10n-id='delete' className='removeBookmarkLink link' onClick={this.onRemoveBookmark} />
: null
}
<Button l10nId='save' className='primaryButton' onClick={this.onSave} />
<Button l10nId='save' className={this.bookmarkNameValid ? 'primaryButton' : 'primaryButton disabled'} onClick={this.onSave} />
</div>
</div>
</div>
Expand Down
4 changes: 4 additions & 0 deletions less/addEditBookmark.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@
.removeBookmarkLink {
margin-right: auto;
}

.disabled {
background-color: #ccc !important;
}