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.

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? #166

Closed
pdehaan opened this issue Mar 13, 2018 · 12 comments
Closed

Dedupe saved themes? #166

pdehaan opened this issue Mar 13, 2018 · 12 comments

Comments

@pdehaan
Copy link
Contributor

pdehaan commented Mar 13, 2018

Steps to reproduce:

  1. Go to https://mozilla.github.io/Themer/
  2. Click the [Save] button on your theme a few dozen times.

Actual results:

A few dozen duplicate themes in your "Saved themes" section.

Expected results:

Maybe we should disallow people to save a theme if they have the same hash already saved in their saved themes.

@cedricium
Copy link
Contributor

I came across the same issue and would agree, only having unique themes saved seems like a good idea. I wouldn't mind tackling this as part of my contribution for the Outreachy program.

I just wonder: if attempting to save a non-unique theme, should it fail silently or should there be some sort of notification / error? @lmorchard @johngruen

@johngruen
Copy link
Contributor

Hey @cedricium I think you could just pop the theme the user is trying to dupe to the front of the saved themes list and then add a little highlight animation to it. probably fading in then out the same box shadow we show on hover would work well. If that's not strong enough we could think about adding a warning, but i bet that would do it.

@cedricium
Copy link
Contributor

@johngruen great idea! Will see what I can do 👍

@Ekta3012
Copy link

Hello @pdehaan , i want to solve this issue as a part of outreachy programme. please, assign it to me.

@cedricium
Copy link
Contributor

Hey @Ekta3012, I was working on this but was having trouble matching an already saved theme with the theme that is going to get saved. Definitely go ahead and give it shot, I'd love to see your solution.

@gargshruti30
Copy link

@cedricium can i work on this issue as a part of my Outreachy program ?
Please assign it to me I really want to work upon this issue.

@cedricium
Copy link
Contributor

@gargshruti30 I'm an outreachy participant as well, not a member of the Themer group. Maybe go ahead and see if you can come up with a solution and think about making a PR afterwards.

@chuckharmston
Copy link

@gargshruti30 consider yourself assigned!

@chuckharmston chuckharmston added the contrib: assigned For issues being worked on by community members label Mar 16, 2018
@lmorchard
Copy link
Contributor

having trouble matching an already saved theme with the theme that is going to get saved.

For what it's worth, we have a small utility function here used for matching themes:

https://github.com/mozilla/Themer/blob/master/src/lib/store.js#L39

It's a hack and should be moved to lib/themes.js if it gets used outside store.js. But, it's what we use to compare an incoming shared theme with the current set theme to decide whether the "accept this shared theme" dialog should be shown

@pdehaan
Copy link
Contributor Author

pdehaan commented Mar 16, 2018

For what it's worth, we have a small utility function here used for matching themes:

Fascinating! Is the order of JSON.stringify() guaranteed? I bit of quick Googling Yahoo!ing left me mildly confused.
auth0/node-jsonwebtoken#404 points to nodejs/node#15628, which seems to imply that guaranteed ordering doesn't seem like it belongs in core (for Node.js, maybe each browser vendor's implementation can differ -- but I guess things may be the same if you're always using the same browser). But then it also points to substack's excellent looking json-stable-stringify module:

json-stable-stringify: deterministic version of JSON.stringify() so you can get a consistent hash from stringified results.

@gargshruti30
Copy link

@lmorchard I will be sure to check that.

@johngruen johngruen added stretch contrib: good first bug skill:js and removed contrib: assigned For issues being worked on by community members labels Apr 3, 2018
@ghost ghost mentioned this issue Apr 14, 2018
@ghost
Copy link

ghost commented Apr 14, 2018

Raised a PR to avoid duplicating themes. #231
Need help to hack the highlight animation effect suggested by @johngruen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants