This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Refactoring, Bug fixes, and tests for top sites on about:newtab #5583
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
Bookmarking items does not affect position Fixes #5413 Sites are ordered by most visited (count) DESC Fixes #5322 Groundwork laid for #5565, but the task is unfinished as-is. ----- These commits moving existing logic from newtab.js into the session helper. Tests were then added and I manually tested each scenario and tried to make sure tests cover those. Things which are not covered are marked with TODO(bsclifton) The important thing: appState.about.newtab.sites no longer persists a separate copy of the sites array. It instead uses the location/partion of items saved to lookup the real object from appState.sites. ----- Auditors: @cezaraugusto, @bbondy
Last remaining known issue(s): 1. ignoring sites - pin some items (position 1, position 5) - ignore by hitting X on item 2 or 3 - items become unpinned :( 2. default top sites - we need to populate this (like we talked about) - brave.com, etc
|
If possible, grab this code and give it a go 😄 Let's find any bugs! I documented two known issues in last commit |
|
++ I'm going to merge and test on master but we can iterate from there for things. |
This was referenced Nov 14, 2016
|
Please specify QA steps if any. |
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).Bookmarking items does not affect position
Fixes #5413
Sites are ordered by most visited (count) DESC
Fixes #5322
Groundwork laid for #5565,
but the task is unfinished as-is.
notes
These commits moving existing logic from newtab.js into the session helper.
Tests were then added and I manually tested each scenario and tried to make
sure tests cover those. Things which are not covered are marked with TODO(bsclifton)
The important thing:
appState.about.newtab.sites no longer persists a separate copy of the sites array.
It instead uses the location/partion of items saved to lookup the real object from appState.sites.
Known issue: ignoring sites does not work properly (we can hide the button as a quick fix or I can resolve before we accept this PR)
Even without de-duping, this patch may get us in good enough shape to ship 0.12.9 😄
Auditors: @cezaraugusto, @bbondy