-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 syntax highlight check #290
Conversation
readOnly: true, | ||
mode: 'javascript' | ||
}, | ||
code: ' function iamHappy(happy) {\n\tif(happy){\n\t console.log("I am Happy!")\n\t}else{\n\t console.log("I am not Happy!")\n\t}\n};' |
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.
[question] Does code
need to be kept by state?
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.
Please place white space before and after else
, if
.
@@ -41,7 +57,23 @@ class UiTab extends React.Component { | |||
lineNumber: this.refs.previewLineNumber.checked | |||
} | |||
|
|||
|
|||
|
|||
let newOptions = { |
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.
Maybe you can use const
.
Basically, please use const
.
} | ||
|
||
this.setState({options: newOptions}) | ||
|
||
this.setState({ config }) |
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.
You can write like below.
this.setState({
options: newOptions, config,
})
@@ -12,12 +13,27 @@ class UiTab extends React.Component { | |||
super(props) | |||
|
|||
this.state = { | |||
config: props.config | |||
config: props.config, | |||
options: { |
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.
options
is fuzzy word. How about codeMirrorOptions
?
lineNumbers: true, | ||
theme: props.config.editor.theme, | ||
readOnly: true, | ||
mode: 'javascript' |
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.
I think lineNumbers
, readOnly
, mode
won't be changed.
You don't need to keep them in state
.
let { config, options } = this.state | ||
let checkHighLight = document.getElementById('checkHighLight') | ||
|
||
if (checkHighLight == null) { |
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.
Please use ===
. ==
or !=
is too fuzzy.
config: props.config, | ||
options: { | ||
lineNumbers: true, | ||
theme: props.config.editor.theme, |
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.
theme
attribute looks not necessary for CodeMirror
component.
https://github.com/JedWatson/react-codemirror/blob/master/src/Codemirror.js#L13-L23
@@ -12,36 +13,53 @@ class UiTab extends React.Component { | |||
super(props) | |||
|
|||
this.state = { | |||
config: props.config | |||
config: props.config, | |||
codemirrorTheme: props.config.editor.theme |
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.
So good! Simple state👼
} | ||
} | ||
|
||
handleUIChange (e) { | ||
let { config } = this.state | ||
let { codemirrorTheme } = this.state |
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.
You can use const
🐶
const newCodemirrorTheme = this.refs.editorTheme.value | ||
|
||
if (newCodemirrorTheme !== codemirrorTheme) { | ||
checkHighLight.setAttribute('href', '../node_modules/codemirror/theme/' + newCodemirrorTheme + '.css') |
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.
Using template literal is more readable.
checkHighLight.setAttribute('href', `../node_modules/codemirror/theme/${newCodemirrorTheme}.css`)
@@ -113,6 +131,9 @@ class UiTab extends React.Component { | |||
}) | |||
} | |||
</select> | |||
<div styleName='code-mirror'> | |||
<CodeMirror value={codemirrorSampleCode} options={{ lineNumbers: true, readOnly: true, mode: 'javascript', theme: codemirrorTheme }} /> |
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.
Good component! Readable 🍰
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.
Please fix some points.
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.
Great work! Thanks for many fixing 👍
Change Editor Theme Setting Screen
Before, when we selected editor theme in UI setting screen, we cannot check real syntax highlight.
After, when we selected editor theme in UI setting screen, we can check real syntax highlight.
Sorry
sometimes , javascript mode is not working. If you do not mind, please tell me the solution.