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

[WIP] Add font family to preferences #78

Merged
merged 6 commits into from
Jul 23, 2017

Conversation

viankakrisna
Copy link
Contributor

Add font family for preference. AFAIK this should be all from the client side, haven't got to test it yet until we have staging server :)

Also, why do we need to injectGlobal here @CompuIves ? I think we can move it to CodeContainer?

@CompuIves
Copy link
Member

Really nice, I was able to get 'Arial' in my code editor hahaha. Okay great, I had to add one preference option to work with strings (only supported boolean and number until now), check this commit: 9942d3f.

Would be great to move the styles to CodeContainer, I never liked injectGlobal very much.

@CompuIves
Copy link
Member

Can you also add a description underneath the Preference option?

@viankakrisna viankakrisna force-pushed the preferences/fontFamily branch from 6a0eedd to 4a00f09 Compare July 23, 2017 17:33
@viankakrisna
Copy link
Contributor Author

ah yes, I'm not aware that it's only accepted boolean and number. 😄 Updated with 9942d3f, description, and the moving of style to CodeContainer

@viankakrisna viankakrisna force-pushed the preferences/fontFamily branch from 4a00f09 to 617fe66 Compare July 23, 2017 17:44
@CompuIves
Copy link
Member

Haha, a coincidence. I'm currently implementing Prettier Preferences and now need the Preference type refactor too. Let me take a look.

@CompuIves
Copy link
Member

Looks good! I made one small change to add specify a placeholder ('Source Code Pro') in the input, makes it more clear on what to input. I think that's enough for the user to see what needs to happen, so we can remove the description.

I added the last required commit here: bf114d8.

And this is how it looks like:
screen shot 2017-07-23 at 20 11 21

Then it'll be ready to merge! Thanks a lot for this PR, this is very valuable.

@CompuIves
Copy link
Member

Great! Let's ship this :shipit: 🚀

@CompuIves CompuIves merged commit 6007305 into codesandbox:master Jul 23, 2017
@CompuIves
Copy link
Member

Oh oops, forgot to change the name. Well, 🤷‍♂️

@viankakrisna
Copy link
Contributor Author

😅 you move really fast! thank you!

@CompuIves
Copy link
Member

Haha, thanks. I'm thinking of replacing the preferences system for something that takes less boilerplate, now you need to change 4 things before it works. Very confusing and tedious.

@viankakrisna
Copy link
Contributor Author

yup, I missed some things when I first opened the PR. Today I just use this for forms shameless plug plain react and lodash 😄

@CompuIves
Copy link
Member

Hmm, interesting. If we could convert the preferences to something like that it would be really nice.

Best case would be if we only had to define preferences in the store, and then it automatically gets synced with localStorage when updated.

@viankakrisna
Copy link
Contributor Author

maybe under one action? SET_PREFERENCE, and just validate the preference schema for that action payload.

@viankakrisna viankakrisna deleted the preferences/fontFamily branch July 23, 2017 19:49
@CompuIves
Copy link
Member

Yap, that's even better.

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.

2 participants