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

EuiCodeEditor: make it take a custom mode #935

Merged
merged 6 commits into from
Jun 20, 2018

Conversation

ycombinator
Copy link
Contributor

Resolves #933.

Allows the mode prop in the EuiCodeEditor component to take an object representing a custom Ace mode.

To test this PR

  • Run this PR along with EUIFication: Grok Debugger kibana#20027.

  • In Kibana, open Dev Tools > Grok Debugger

  • In the Grok Pattern field (which uses a custom mode), enter:

    %{WORD:foo}
    
  • Verify that the syntax highlighting changes once you finish typing.

/**
* Use string for a built-in mode or object for a custom mode
*/
mode: PropTypes.oneOfType([
Copy link
Contributor

Choose a reason for hiding this comment

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

When passing mode as an object, react-ace's own proptype fails: Failed prop type: Invalid prop mode of type object supplied to ReactAce, expected string. The code editor's render function should be updated to pass mode as undefined if it is custom.

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 in 8b6523e.

this.aceEditor.editor.getSession().setMode(this.props.mode);
}

componentDidMount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should also add componentDidUpdate and compare old props.mode with new props.mode, updating the session if the old & new modes differ and if the new mode is a custom object (switching to a string is handled internally by react-ace).

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 in 1b1b3db.

@ycombinator
Copy link
Contributor Author

@chandlerprall Thanks for catching those issues. I've fixed both of them now and this PR is ready for your 👀 again.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM; pulled branch & debugged the existing and new code editor examples to ensure values & program flow were as expected.

@chandlerprall
Copy link
Contributor

Thanks @ycombinator for the change!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

ha, real quick could you modify the CHANGELOG.md to include this feature add?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

still LGTM, changelog was part of the original PR... I was blind

@ycombinator ycombinator merged commit fae538f into elastic:master Jun 20, 2018
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