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.

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

Add context menu to TagTile #1743

Merged
merged 3 commits into from
Feb 13, 2018

Conversation

lukebarnard1
Copy link
Contributor

With two options: View Community and Remove, which
removes the tag from the panel.

With two options: View Community and Remove, which
removes the tag from the panel.
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

lgtm other than n^2 worry (and that TagOrderStore is now storing more than just tag ordering)

return () => {};
}

removedTags.push(tag);
Copy link
Member

Choose a reason for hiding this comment

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

Could you do all this computation (ie. all the function up until this point) in the async action? That way if you have two in flight at once, you don't lose an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, we need to calculate removedTags here for the optimistic update.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see - ok, so having traced through how all the async actions and actions creators work again, the contents of the store gets updated as soon as the event dispatch gets around the event loop (ie. once the pending lands), so the window where you could lose the update is fairly minimal. (Actually I think its the same array object that gets updated each time anyway so possibly it doesn't race at all...)

I guess this is OK then.

@@ -165,13 +175,15 @@ class TagOrderStore extends Store {
_mergeGroupsAndTags() {
const groupIds = this._state.joinedGroupIds || [];
const tags = this._state.orderedTagsAccountData || [];
const removedTags = this._state.removedTagsAccountData || [];
Copy link
Member

Choose a reason for hiding this comment

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

Feels like we may want to turn this into a set (or even store it on the store as a set) to make the membership testing less O(n^2).

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Feb 13, 2018
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Feb 13, 2018
@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Feb 13, 2018
@lukebarnard1
Copy link
Contributor Author

lukebarnard1 commented Feb 13, 2018

The tests are failing and it's a Real Failure! 😀

Never mind, apparently it's some weird combination of using the unmerged branch of the other PR in combination with the merged branch of this one. Can haz one project?

@lukebarnard1 lukebarnard1 merged commit c670b76 into develop Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants