fixes #24380 - load bookmarks on demand #5872
Conversation
This commits change that bookmarks are fetched (on every index action) only once the bookmarks dropdown is opneded, this avoids extra API call on most index pages when users don't use bookmarks.
|
Issues: #24380 |
|
ping @theforeman/ui-ux for a quick review. /cc @LaViro |
|
@amirfefer adding prop type, changing directory structure (to align to best practices etc) imho is out of scope for this PR and should be done for all existing components? |
|
@ohadlevy You can add propTypes without changing the folder structure and we will still get benefits from it. |
|
@tstrachota got a minute for a quick review? @sharvit @amirfefer and I agreed that refactoring for all components should be in another PR... |
| componentDidMount() { | ||
| const { url, controller, getBookmarks } = this.props; | ||
| loadBookmarks() { | ||
| if (this.props.bookmarks.length === 0 && this.props.status !== STATUS.PENDING) { |
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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. 😃
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@ripcurld any suggestions ?
There was a problem hiding this comment.
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.
|
PR it please :)
…On Mon, Aug 6, 2018 at 12:32 PM ripcurld ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In webpack/assets/javascripts/react_app/components/bookmarks/index.js
<#5872 (comment)>:
> }
- componentDidMount() {
- const { url, controller, getBookmarks } = this.props;
+ loadBookmarks() {
+ if (this.props.bookmarks.length === 0 && this.props.status !== STATUS.PENDING) {
@ohadlevy <https://github.com/ohadlevy> I think I worked it out. See here
<ripcurld/foreman@c713d96>
.
Let me know what you think.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5872 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABOx2vMrqqG2ntkFL6oCUKelZxDTxAFks5uOA1IgaJpZM4VfvzQ>
.
|
This commits change that bookmarks are fetched (on every index action)
only once the bookmarks dropdown is opneded, this avoids extra API
call on most index pages when users don't use bookmarks.