-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature/code mirror theme exposed #323
Feature/code mirror theme exposed #323
Conversation
See discussion in #129 |
Less is more. We can remove these words without subtracting any meaningful detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me overall, but before we merge, I want to ask whether just "theme" would be a better name for this prop. The fact that the editor views are powered by CodeMirror is really just an implementation detail, and using codeMirror
in the prop name leaks that out. In the future, if a better implementation comes along, maybe we want to preserve the ability to pass in themes like "solarized" to it (obviously, this would depend on the appropriate CSS being available, so you could argue that changing the implementation would be a breaking change and we therefore should pick a different property name; but I do like the idea of keeping some clutter out of the API).
Thoughts?
@@ -9,7 +9,6 @@ | |||
/* eslint-disable no-console */ | |||
import express from 'express'; | |||
import path from 'path'; | |||
import fs from 'fs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naughty! Sneaking in an unrelated change.
Oh, forgot to mention: I applied some small fixes directly on the PR branch rather than incurring another round trip. If you do more work on this, be sure to pull those down. |
This is a great point, and something that I thought about in some capacity. Here's my thoughts:
I think maybe Would that be ok @wincent? |
Or maybe Agreed that for all practical purposes CodeMirror is likely to remain the implementation, but that doesn't mean we shouldn't try to practice good hygiene and abstract over it. |
💥 |
…m/joelgriffith/graphiql into feature/code-mirror-theme-exposed
Looks good. Thanks for your work on this! |
Thanks for catching that JSX error....again! |
First pass on exposing a
codeMirrorTheme
prop, plumbing it through, and docs on how to load/use css for the theme.