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

Replace cookies with localStorage #70

Merged
merged 1 commit into from Feb 21, 2016
Merged

Replace cookies with localStorage #70

merged 1 commit into from Feb 21, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 18, 2016

Fixes #69. We don't need to store any of the data on cookies, localstorage is better suited for this.

@xPaw xPaw added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed labels Feb 18, 2016
@astorije
Copy link
Member

It's rather a non-minor change, I'm very much in favor of having 2 reviews for this (not that I am against this, quite the opposite, but 2 reviews rather than 1 seem safe and sensible to me here :-) ). Removing the second review needed for now.

@AlMcKinlay AlMcKinlay self-assigned this Feb 19, 2016
@AlMcKinlay
Copy link
Member

At a first pass, this looks good. I'll take a more detailed look later, and run it to make sure it works. Looks good so far though, and nothing to comment as of yet.

@AlMcKinlay
Copy link
Member

👍 Looks good, seems to work fine.

@astorije
Copy link
Member

This is really great indeed, 👏 @xPaw!

One thing that might be more relevant in the future than this PR, but still worth noting: by releasing this, everyone currently with specific settings will see their settings reset to default.
Avoiding that or not is a trade-off: having code that defaults to reading from the cookie that or not. Drawback is of course that this code will be less and less relevant over time...
Considering that this fork is pretty new, our current userbase is small and the amount of settings is too, so I think we can simply ignore that for now and simply add a notice on the change log / release information. However, this is something we should keep in mind next time we have a similar change on bigger legacy.

What do you think?

@xPaw
Copy link
Member Author

xPaw commented Feb 21, 2016

Yeah, as our fork is rather new, I wouldn't want to introduce backwards compatibility like this just yet.

@AlMcKinlay
Copy link
Member

Yeah, I agree with that. I think it's a little annoying in the short term, but not enough as it's really not hard to reset your settings once as there aren't that many settings.

I wouldn't want to invalidate the settings again, but this once at the beginning of our fork sounds fine to me :-)

@astorije
Copy link
Member

Agreed with both of you. I'm thinking we might have to do that later on though: when introducing packages, we might want to namespace settings to avoid packages playing around with settings from other packages. But a fight for another day I guess :-)

👍 and merging.

astorije added a commit that referenced this pull request Feb 21, 2016
Replace cookies with localStorage
@astorije astorije merged commit f8d8b74 into thelounge:master Feb 21, 2016
@AlMcKinlay
Copy link
Member

I'm thinking we might have to do that later on though: when introducing packages, we might want to namespace settings to avoid packages playing around with settings from other packages.

Possibly, yeah. We can take that as it comes along :-)

@astorije astorije assigned AlMcKinlay and unassigned AlMcKinlay Feb 25, 2016
@xPaw xPaw deleted the localstorage branch March 7, 2016 17:17
@astorije astorije added this to the 1.2.0 milestone Apr 1, 2017
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants