Dedupe saved themes #231
Dedupe saved themes #231
Conversation
ghost
mentioned this pull request
src/web/lib/storage.js
Outdated
| for (const key of Object.keys(themesList)) { | ||
| isThemeDuplicate = themesEqual(theme, themesList[key].theme); | ||
| if (isThemeDuplicate) { | ||
| callback(isThemeDuplicate); |
There was a problem hiding this comment.
From the looks of this, wouldn't the callback be called twice here? Once on L42 if a match was found, and again on L46 after the break statement breaks out of the for..of loop.
There was a problem hiding this comment.
That's a nice catch. Thanks.
I pushed the latest changes, please review it.
src/web/lib/storage.js
Outdated
| ); | ||
| notifySelfForStorage(storageKey); | ||
| checkDuplicateTheme(theme, (isThemeDuplicate) => { | ||
| if (!isThemeDuplicate) { |
There was a problem hiding this comment.
Since localStorage.setItem is syncronous, you don't need a callback here. You can just return the result from isThemeDuplicate
There was a problem hiding this comment.
Thank you. I presumed it was asynchronous. Will make this change.
src/web/lib/storage.js
Outdated
| if (isThemeDuplicate) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This could be written a little more cleanly, maybe something like:
return Object.keys(listThemes()).filter((key) => {
return themesEqual(theme, themesList[key].theme);
}).length > 0;There was a problem hiding this comment.
@meandavejustice: Wouldn't we want something like Array#some() [instead of Array#filter()] which should exit the loop as soon as a match is found?
There was a problem hiding this comment.
@pdehaan Sure thing, some is even better!
There was a problem hiding this comment.
Thanks for this suggestion too. I learnt a new hack.
src/web/lib/storage.js
Outdated
|
|
||
| function checkDuplicateTheme(theme) { | ||
| const themesList = listThemes(); | ||
| return Object.keys(listThemes()).some((key) => { |
There was a problem hiding this comment.
Sorry for being annoying...
Small nit, the listThemes() here should be themesList from L36 above, so we aren't calling the method twice.
There was a problem hiding this comment.
Ahh, I missed that one. Thanks. Will push the changes.
src/web/lib/storage.js
Outdated
| }) | ||
| ); | ||
| notifySelfForStorage(storageKey); | ||
| } |
There was a problem hiding this comment.
Semi-related, but do we need an else statement?
Re-reading @johngruen's #166 (comment), it sounded like we may want to pop the newly saved duplicate theme to the front of the list if it was a duplicate. Not sure if the code is currently doing that and I'm just not reading it correctly, or if we need an else where we update the modified date to Date.now() and do whatever animations we wanted.
There was a problem hiding this comment.
Since the functionality is the main focus here I did not make changes for animation effect. I need help with that. I'm still figuring out how to invoke the animation in SavedThemeSelector component when Save button is clicked. Could you please give a hint on how to proceed with that?
There was a problem hiding this comment.
Can we merge this code and open a new issue to include animation effect?
There was a problem hiding this comment.
Hi @meandavejustice that sounds good.
While testing the changes after resolving the conflicts, I found that 'Save' button is disabled if there is no change from 'currentSaved' theme.
One case where this functionality (of checking duplicate saved themes) would be useful is when a user presses 'undo/redo' button and tries to save the same theme again.
So, would you still recommend to add this functionality?
Please let me know if I'm missing something here.
There was a problem hiding this comment.
@pdehaan @meandavejustice Could you please review if we still need to fix this issue?
If not, we can close this PR.
|
Cleaned up the conflicts and think I'm going to merge this. There's some UI polish we can add on top of this, but it works as a start! |
It Fixes #166
Disallows to save a theme, if it is present in the saved theme already.
@pdehaan @johngruen please review this.