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

fix issue #321 - load defaults, then localStorage, then url config #322

Merged
merged 2 commits into from
Apr 11, 2018
Merged

fix issue #321 - load defaults, then localStorage, then url config #322

merged 2 commits into from
Apr 11, 2018

Conversation

stevensacks
Copy link
Contributor

Description

For IDEs that allow you to open selected code in Carbon, they only pass the language and the code. To avoid losing your localStorage config, the settings should be merged in the following order:

  1. DEFAULT
  2. localStorage
  3. urlParams

This ensures that all required config is present and that url params trump all.

}
}
this.setState({
...this.state,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to include this.state in setState (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp. Haha. Yeah you're right. One sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mfix22
Copy link
Contributor

mfix22 commented Apr 10, 2018

@stevensacks thanks so much no idea why I didn't write it like this the first time. Way better.

@briandennis see any issues?

@stevensacks
Copy link
Contributor Author

stevensacks commented Apr 10, 2018

Second set of eyes always helps. :)

Great tool! Happy to help make it just a little bit better. Now, I want to add Darcula theme (you have Dracula, but Darcula is better, IMO).

@mfix22 mfix22 merged commit 9e519e2 into carbon-app:master Apr 11, 2018
@stevensacks
Copy link
Contributor Author

Let me know when it's deployed!

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

Successfully merging this pull request may close these issues.

3 participants