Skip to content
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.

Sign up for GitHub

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

fixes #24380 - load bookmarks on demand #5872

Merged
merged 1 commit into from
Aug 6, 2018
Merged

fixes #24380 - load bookmarks on demand #5872

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
24 changes: 24 additions & 0 deletions ...ck/assets/javascripts/react_app/components/bookmarks/__snapshots__/bookmarks.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@ exports[`bookmarks empty state 1`] = `
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -48,10 +50,12 @@ exports[`bookmarks empty state 1`] = `
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down Expand Up @@ -315,12 +319,14 @@ exports[`bookmarks should include existing bookmarks for the current controller
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -329,10 +335,12 @@ exports[`bookmarks should include existing bookmarks for the current controller
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down Expand Up @@ -701,12 +709,14 @@ exports[`bookmarks should not allow creating a new bookmark for users who dont h
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -715,10 +725,12 @@ exports[`bookmarks should not allow creating a new bookmark for users who dont h
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down Expand Up @@ -1065,12 +1077,14 @@ exports[`bookmarks should show an error message if loading failed 1`] = `
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -1079,10 +1093,12 @@ exports[`bookmarks should show an error message if loading failed 1`] = `
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down Expand Up @@ -1372,12 +1388,14 @@ exports[`bookmarks should show loading spinner when loading bookmarks 1`] = `
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -1386,10 +1404,12 @@ exports[`bookmarks should show loading spinner when loading bookmarks 1`] = `
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down Expand Up @@ -1650,12 +1670,14 @@ exports[`bookmarks should show no bookmarks if server did not respond with any 1
>
<Uncontrolled(Dropdown)
id="hosts"
onClick={[Function]}
pullRight={true}
>
<Dropdown
bsClass="dropdown"
componentClass={[Function]}
id="hosts"
onClick={[Function]}
onToggle={[Function]}
pullRight={true}
>
Expand All @@ -1664,10 +1686,12 @@ exports[`bookmarks should show no bookmarks if server did not respond with any 1
bsClass="btn-group"
className="dropdown"
justified={false}
onClick={[Function]}
vertical={false}
>
<div
className="dropdown btn-group"
onClick={[Function]}
>
<DropdownToggle
bsClass="dropdown-toggle"
Expand Down
12 changes: 7 additions & 5 deletions webpack/assets/javascripts/react_app/components/bookmarks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ import helpers from '../../common/helpers';
class BookmarkContainer extends React.Component {
constructor(props) {
super(props);
helpers.bindMethods(this, ['handleNewBookmarkClick']);
helpers.bindMethods(this, ['handleNewBookmarkClick', 'loadBookmarks']);
}

componentDidMount() {
const { url, controller, getBookmarks } = this.props;
loadBookmarks() {
if (this.props.bookmarks.length === 0 && this.props.status !== STATUS.PENDING) {
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid the logic under if will grow.
what about:

const {bookmarks, status, url, controller, getBookmarks} = this.props;
if (bookmarks.length > 0 || status === STATUS.PENDING) {
  // do nothing: bookmarks are being loaded or already loaded earlier
  return ;
}
getBookmarks(url, controller)

In addition, should we add tests?

Other than that LGTM 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

a. I agree the statement might grow, but if it does, we will change as needed?
b. I didnt extract the values just to reduce the amount of const extractions in case its a noop - don't mind changing but ymmv
c. regarding tests, whats the best way to test (now that I'm a bit pushed away from the code)? I can see test that onclick calls the function, and that the condition are tested (e.g. if the action is fired or not)?

Copy link
Member

Choose a reason for hiding this comment

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

I tried to write a test to BookmarksContainer but I realized that the class itself isn't exported and thus getting access to getBookmarks/loadBookmarks is not trivial as I though. 😮

Regarding a and b - you're probably right. 😃

Copy link
Member

Choose a reason for hiding this comment

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

Um, interesting, I actually got access to them but it feels like it's not responding to my click event. I will try to investigate more.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ripcurld any suggestions ?

Copy link
Member

Choose a reason for hiding this comment

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

nope but don't want to block this either. I tested it manually and it works 👍 .

If I figure a way to test it, I will send a pull request.

Copy link
Member

Choose a reason for hiding this comment

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

@ohadlevy I think I worked it out. See here.

Let me know what you think.

const { url, controller, getBookmarks } = this.props;

getBookmarks(url, controller);
getBookmarks(url, controller);
}
}

handleNewBookmarkClick() {
Expand All @@ -45,7 +47,7 @@ class BookmarkContainer extends React.Component {
return showModal ? (
<SearchModal show={showModal} controller={controller} url={url} onHide={modalClosed} />
) : (
<Dropdown pullRight id={controller}>
<Dropdown pullRight id={controller} onClick={this.loadBookmarks}>
<Dropdown.Toggle title={__('Bookmarks')}>
<Icon type='fa' name='bookmark' />
</Dropdown.Toggle>
Expand Down