Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upChange sites from List to Map #5587
Conversation
| @@ -439,10 +439,15 @@ const handleAppAction = (action) => { | |||
| appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), s, action.tag)) | |||
| }) | |||
| } else { | |||
| appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), action.siteDetail, action.tag, action.originalSiteDetail)) | |||
| let sites = appState.get('sites') | |||
| if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) { | |||
This comment has been minimized.
This comment has been minimized.
darkdh
Nov 14, 2016
Author
Member
This is the root cause of last backed out
766722a#diff-23ca389e2bcb77191b5a9c10900eb3a3R443
|
Because this can cause issues with migrations, I wanted to propose a new data-structure for us to use in favor of the sites array and ask what everyone thinks? We could still keep the migration code in there (so that it copies the old values over to the new structure), but instead of: |
|
This needs to be rebased |
|
Still needs to be rebased, I'll move to 1.0 |
|
rebased! but still looking for the cause of multi-select on bookmark manager failure |
darkdh
force-pushed the
sites-refactor
branch
2 times, most recently
from
2873ec9
to
9555a85
|
It's ready for review again |
darkdh
force-pushed the
sites-refactor
branch
2 times, most recently
from
e3f4c76
to
5fdfdcb
|
@darkdh please rebase |
|
@posix4e , rebasing complete! |
|
@darkdh sorry to be a bother but how was this tested? |
|
@darkdh when you're done can you provide some bulk bookmark file as your STR suggested? Mine don't reach 300 |
|
sent on slack |
|
Tested manually and everything worked as expected. However, webDriver tests didn't pass for me for Update: Intermittent fail. Tested again and it worked. |
|
Previous comment pointed to a failing on |
|
@darkdh I think this needs another rebase if you have time between C56 building. |
fix #4879 Auditors: @bbondy, @bsclifton, @cezaraugusto Test Plan: Performance: 1. Import bulk bookmarks (4000+) from other browsers (Don't merge into toolbar) 2. The process should finish instantly 3. Delete Import from XXX folder 4. The process should finish instantly Migration: 1. Make sure session-store-1 contains the sites data before 0.12.10 2. Lauch Brave and Close 3. sites of session-store-1 should change from [] tp {}
|
@bbondy, rebased |
luixxiul
added
PR/needs-QA-attention
|
++ |
|
It's nice to see this finally merged |
darkdh commentedNov 14, 2016
•
edited by luixxiul
git rebase -ito squash commits (if needed).Test Plan:
fix #4879
Auditors: @bbondy, @bsclifton, @cezaraugusto
Test Plan:
Moved to #4879 (comment)