This repository has been archived by the owner. It is now read-only.
feat (systemaddon): #3147 implement highlights section #3369
Conversation
| @@ -203,12 +203,16 @@ function Sections(prevState = INITIAL_STATE.Sections, action) { | |||
| let order; | |||
| let index; | |||
| if (prevState.length > 0) { | |||
| order = action.data.order || prevState[0].order - 1; | |||
| order = action.data.order !== undefined ? action.data.order : prevState[0].order - 1; | |||
k88hudson
Sep 7, 2017
Member
Can you add a test for this?
Can you add a test for this?
|
First screenshot! |
|
@Mardak ok, I think I have it covered decently with tests now. not sure why travis isn't running |
rlr
changed the title
|
|
||
| // Remove any Highlights that are in Top Sites already | ||
| const topsites = this.store.getState().TopSites.rows.map(site => site.url); | ||
| this.highlights = this.highlights.filter(site => topsites.indexOf(site.url) === -1); |
Mardak
Sep 7, 2017
•
Member
We could use Dedupe here. #3379 Or this is fine for now. Interestingly, because we update highlights when there aren't enough results but not for top sites, recently visited what-would-be-top-site appears as a highlight
We could use Dedupe here. #3379 Or this is fine for now. Interestingly, because we update highlights when there aren't enough results but not for top sites, recently visited what-would-be-top-site appears as a highlight
| id: "highlights", | ||
| pref: { | ||
| titleString: {id: "settings_pane_highlights_header"}, | ||
| descString: {id: "settings_pane_highlights_body2"} |
Mardak
Sep 7, 2017
Member
I'm guessing section manager doesn't currently support the complex sub pref options, so we can split that out to fully fix #3155.
I'm guessing section manager doesn't currently support the complex sub pref options, so we can split that out to fully fix #3155.
| break; | ||
| case at.PLACES_BOOKMARK_ADDED: | ||
| case at.PLACES_BOOKMARK_REMOVED: | ||
| this.fetchHighlights(false); |
Mardak
Sep 7, 2017
•
Member
I see that we avoid updating the UI here. But the context menu doesn't update either. Perhaps the Section Reducer isn't working correctly? #3378
I see that we avoid updating the UI here. But the context menu doesn't update either. Perhaps the Section Reducer isn't working correctly? #3378
| case at.NEW_TAB_LOAD: | ||
| if (this.highlights.length < HIGHLIGHTS_MAX_LENGTH) { | ||
| // If we haven't filled the highlights grid yet, fetch again. | ||
| this.fetchHighlights(true); |
Mardak
Sep 7, 2017
Member
This broadcast means all open pages get the update. This behavior is different from top sites.
This broadcast means all open pages get the update. This behavior is different from top sites.
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.
Fixes #3147
Fixes some of #3155