Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dedupe saved themes #231

Merged
merged 6 commits into from May 8, 2018
Merged

Dedupe saved themes #231

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4185995
issue 166 - dedupe saved themes
csd713 Apr 14, 2018
b860c7f
remove extra callback
csd713 Apr 15, 2018
d1bf227
removed callback
csd713 Apr 17, 2018
ea1e415
remove log line
csd713 Apr 17, 2018
c86af7b
remove extra function call
csd713 Apr 17, 2018
be71154
Merge branch 'master' into issue-166-dedupe-themes
lmorchard May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const actions = {
}
};

const themesEqual = (themeA, themeB) =>
export const themesEqual = (themeA, themeB) =>
// HACK: "deep equal" via stringify
// http://www.mattzeunert.com/2016/01/28/javascript-deep-equal.html
JSON.stringify(themeA) === JSON.stringify(themeB);
Expand Down
32 changes: 21 additions & 11 deletions src/web/lib/storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { actions } from "../../lib/store";
import { actions, themesEqual } from "../../lib/store";
import { makeLog } from "../../lib/utils";
import { normalizeTheme } from "../../lib/themes";

Expand All @@ -17,16 +17,26 @@ const generateThemeKey = () =>
`${Date.now()}-${Math.floor(Math.random() * 1000)}`;

function putTheme(key, theme) {
log("putTheme", key, theme);
const storageKey = themeStorageKey(key);
localStorage.setItem(
storageKey,
JSON.stringify({
theme,
modified: Date.now()
})
);
notifySelfForStorage(storageKey);
let isThemeDuplicate = checkDuplicateTheme(theme);
if (!isThemeDuplicate) {
log("putTheme", key, theme);
const storageKey = themeStorageKey(key);
localStorage.setItem(
storageKey,
JSON.stringify({
theme,
modified: Date.now()
})
);
notifySelfForStorage(storageKey);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this code and open a new issue to include animation effect?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csd713 I'm cool with doing the animation in a separate issue, you wanna open one up with a link to john's comment from #166 ?

Also this needs a rebase after some changes from this morning before I can merge

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdehaan @meandavejustice Could you please review if we still need to fix this issue?
If not, we can close this PR.

}

function checkDuplicateTheme(theme) {
const themesList = listThemes();
return Object.keys(listThemes()).some((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being annoying...
Small nit, the listThemes() here should be themesList from L36 above, so we aren't calling the method twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I missed that one. Thanks. Will push the changes.

return themesEqual(theme, themesList[key].theme);
});
}

function deleteTheme(key) {
Expand Down