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

Add isReadOnly prop to EuiCodeEditor. #169

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

cjcenizal
Copy link
Contributor

  • Place cursor at beginning of code editor if it's read-only.
  • Surface prompt for isReadOnly mode. Only show cursor when the editor is focused.

Ports elastic/kibana#14804

* Place cursor at beginning of code editor if it's read-only.
* Surface prompt for isReadOnly mode. Only show cursor when the editor is focused.
@cjcenizal cjcenizal changed the title Add isReadOnly prop to KuiCodeEditor. Add isReadOnly prop to EuiCodeEditor. Nov 30, 2017
@bevacqua
Copy link
Contributor

Curious, why would you have a readonly editor mode instead of just using a code block?

@cjcenizal
Copy link
Contributor Author

I don't recall... @nreese can you answer Nico's question?

@cjcenizal cjcenizal merged commit 4cd31d9 into elastic:master Nov 30, 2017
@cjcenizal cjcenizal deleted the code-editor-read-only branch November 30, 2017 21:31
@nreese
Copy link
Contributor

nreese commented Nov 30, 2017

@bevacqua The kibana home page - add data needs to present read only instructions with syntax highlighting. A code block does not provide syntax highlighting.

@bevacqua
Copy link
Contributor

Code blocks provide syntax highlighting through a language property:

<EuiCodeBlock language='json'>{ 'var someCode = "foo"' }</EuiCodeBlock>

@cjcenizal can we revisit this and consider reverting the merge? I'd argue we should always try to present code using blocks and not an editor, for consistency.

@bevacqua
Copy link
Contributor

The code editor component is pretty heavyweight so we should strive to use it only when editing is required. cc @snide

@nreese
Copy link
Contributor

nreese commented Nov 30, 2017

@bevacqua Does the code block provide line numbers?

@cjcenizal
Copy link
Contributor Author

cjcenizal commented Nov 30, 2017

@bevacqua I agree with you that there's an argument to be made that we could be duplicating functionality and there is probably a best practice here where we should use the code block instead. However, right now I just want to retain parity with the KUI Framework to ease the migration to EUI. We can't verify your thesis without trying out EuiCodeBlock in Kibana and we can do that by either a) porting it to KUI Framework or b) incorporating EUI Framework into Kibana. We're already in the process of B so I'd rather incur this technical debt temporarily and then remove it later in the process. Does this make sense?

@bevacqua
Copy link
Contributor

@nreese No, you can see live demos here: https://elastic.github.io/eui/#/code, but again, I think consistency is important

@cjcenizal
Copy link
Contributor Author

cjcenizal commented Nov 30, 2017

A temporary solution might be to add your caveat as a CallOut at the top of the CodeEditor doc site page because I think you raise a really great point about consistency.

@bevacqua
Copy link
Contributor

@cjcenizal It's just a prop on the public interface of the EuiCodeBlock, not much of a scientific thesis. FWIW we've been using it extensively in Cloud

@bevacqua
Copy link
Contributor

@cjcenizal If you're already adopting EUI in Kibana, why would going from <EuiCodeEditor language='whatever' readOnly={ true }>{ code }</EuiCodeEditor> to <EuiCodeBlock language='whatever'>{ code }</EuiCodeBlock> be an impediment?

@cjcenizal
Copy link
Contributor Author

@bevacqua There are a few accessibility benefits this component carries with it which CodeBlock doesn't (e.g. you can tab to it and use enter/exit to focus/blur on it as opposed to having the screen-reader automatically start reading it). It also exposes more props and functionality than the CodeBlock component does. Like I said, I think you're probably right about this but I don't think it's a given that we can/will migrate our particular use of the read-only CodeEditor to CodeBlock because they are significantly different components. It's easier for us to maintain parity for now, and try to refactor our consumption of the EUI Framework once we're actually using it.

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.

3 participants