This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Fix gridLayout pinning action bug #5406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix #5337 Auditors: @bsclifton @bbondy /cc @luixxiul @srirambv Our topSites grid have actions connected to each other (pinning/removing/reordering), so we must ensure all of them are working properly while doing multiple actions. Test Plan: * Access new tab page; * Access several websites (you can grab this JSON https://github.com/cezaraugusto/brave-json/blob/master/newtab-3-rows.json and copy/paste on session-store-1, it has 18 sites); * Reordering a tile should work on any position; * Pin a site. Pinning a site should add a Pin icon on top-right corner and be kept on its position (not jump); * Keep in mind the website position and reorder it to a new position; * Access another website and get back to new tab. Ensure that your last pinned website kept its position; * Remove that site (pinned or not); * A modal will open. "Undo" removing; * There should be no pinned icon; * Website should be back to its last position; * Pin several websites; * Reorder pinned websites; * Access a new website; * Go back to new tab page: pinned sites should remmember their positions; * Remove a website on removal modal and hit "restore all"; * All sites must be unpinned.
bsclifton
reviewed
Nov 4, 2016
| if (updatedStamp === this.state.updatedStamp) { | ||
| return | ||
| } | ||
|
|
||
| // Ensure top sites only has unique values |
There was a problem hiding this comment.
Since this logic is removed (which is OK), maybe we can move what's left into ipc.on() line 😄 I originally had added this because my state got into a situation where I had many duplicates, which caused problems
There was a problem hiding this comment.
(can be done as a follow up in a later commit)
|
LGTM! Thanks for sticking this one out and taking the time to manually test each of those scenarios; this page is looking great 😄 |
This was referenced Nov 10, 2016
|
This is not working on Windows. The pinned tabs all moves to the top of the grid after opening a newtab. Will the fix for #5337 fix this as well? |
luixxiul
added
the
needs-info
luixxiul
removed
the
needs-info
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git rebase -ito squash commits (if needed).Fix #5337
Auditors: @bsclifton @bbondy
/cc @luixxiul @srirambv
Our topSites grid have actions connected to each other (pinning/removing/reordering),
so we must ensure all of them are working properly while doing multiple actions.
Test Plan:
and copy/paste on session-store-1, it has 18 sites);