Skip to content
This repository was archived by the owner on May 12, 2026. It is now read-only.

Feature syntax highlight check#290

Merged
sota1235 merged 4 commits into
masterfrom
featureSyntaxHighlightCheck
Apr 22, 2017
Merged

Feature syntax highlight check#290
sota1235 merged 4 commits into
masterfrom
featureSyntaxHighlightCheck

Conversation

@sosukesuzuki

Copy link
Copy Markdown
Contributor

Change Editor Theme Setting Screen

Before, when we selected editor theme in UI setting screen, we cannot check real syntax highlight.
2017-02-17 00 02 52
After, when we selected editor theme in UI setting screen, we can check real syntax highlight.
2017-02-17 00 24 09

Sorry

sometimes , javascript mode is not working. If you do not mind, please tell me the solution.

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};'

@sota1235 sota1235 Feb 17, 2017

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor

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.




let newOptions = {

Copy link
Copy Markdown
Contributor

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 })

@sota1235 sota1235 Feb 17, 2017

Copy link
Copy Markdown
Contributor

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, 
})
this.state = {
config: props.config
config: props.config,
options: {

Copy link
Copy Markdown
Contributor

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'

Copy link
Copy Markdown
Contributor

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) {

Copy link
Copy Markdown
Contributor

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,

Copy link
Copy Markdown
Contributor

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

this.state = {
config: props.config
config: props.config,
codemirrorTheme: props.config.editor.theme

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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')

Copy link
Copy Markdown
Contributor

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`)
}
</select>
<div styleName='code-mirror'>
<CodeMirror value={codemirrorSampleCode} options={{ lineNumbers: true, readOnly: true, mode: 'javascript', theme: codemirrorTheme }} />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good component! Readable 🍰

@sota1235 sota1235 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please fix some points.

@sota1235 sota1235 left a comment

Copy link
Copy Markdown
Contributor

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 👍

@sota1235 sota1235 merged commit c1a76b6 into master Apr 22, 2017
@sota1235 sota1235 deleted the featureSyntaxHighlightCheck branch April 22, 2017 04:20
@sota1235 sota1235 mentioned this pull request Apr 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants