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

fixed readonly property of code-editor #2967

Closed
wants to merge 4 commits into from

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Mar 3, 2020

Summary

Fixes : #2960
fixed readonly property of code-editor

reference : https://github.com/securingsincity/react-ace/blob/master/docs/Ace.md

readOnly should be passed directly to the ace-editor instead of an option as suggested by the documentation

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@myasonik
Copy link
Contributor

myasonik commented Mar 3, 2020

This doesn't seem to fix the problem described in the ticket

(Notice, there's still no [readonly](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly) property on the textarea)
Screen Shot 2020-03-03 at 06 40 47

react-ace isn't the most accessible thing so there isn't an API to do this, we have to mutate the dom after render to do this. (Kibana example, though I did mess up a little, the value of readonly should be readonly not true)

@anishagg17
Copy link
Contributor Author

Screenshot 2020-03-03 at 5 47 36 PM

Fixed it, by updating ace to 8.0.0.
Please recheck @myasonik

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Great, I can see that readonly is now applied!

package.json Outdated Show resolved Hide resolved
src-docs/src/views/code_editor/code_editor.js Outdated Show resolved Hide resolved
@myasonik
Copy link
Contributor

myasonik commented Mar 3, 2020

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2967/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

I think it's a combination of the react-ace version update and the removal of brace from its deps, but syntax validation no longer occurs (compare https://eui.elastic.co/#/forms/code-editor to https://eui.elastic.co/pr_2967/#/forms/code-editor)

It's possible swapping the brace import statements for ace-build imports will solve it, but that may constitute a breaking change for consumers. Was a readOnly feature the reason for the version update, or can we continue to use [email protected] and reinstate the brace imports?

@myasonik
Copy link
Contributor

myasonik commented Mar 3, 2020

Without updating react-ace the workaround to get the readonly property is to do a post-render DOM update

@anishagg17
Copy link
Contributor Author

@myasonik I am trying to restore highlighting

@anishagg17
Copy link
Contributor Author

@thompsongl I have fixed syntax validation ,highlight

@anishagg17
Copy link
Contributor Author

Was a readOnly feature the reason for the version update, or can we continue to use [email protected] and reinstate the brace imports?

we should go with newer version as it major part was Removing brace as a dependency for ace-builds, which builds it's own integrated styles,supports
https://github.com/securingsincity/react-ace/blob/master/CHANGELOG.md

@thompsongl
Copy link
Contributor

We'll need to label this as a breaking change because of the brace to ace-build dependency change. Kibana (and other consumers) has brace import statements alongside usage of EuiCodeEditor.
@chandlerprall can you think of any other potential impacts here?

@chandlerprall
Copy link
Contributor

brace vs. ace-build could have unintended side effects in:

It's been too long since I've looked into the differences in the various ace packages, so I don't know what I may be mis-attributing to another package, and we'd need a full exploration of these changes to know.

Another option, given:

Without updating react-ace the workaround to get the readonly property is to do a post-render DOM update

is related to #2961 where we're already exploring dynamic attribution setting after mount

@myasonik
Copy link
Contributor

myasonik commented Mar 4, 2020

Maybe we just do this as a post-render update and table the package update until we can do a full test that it doesn't break anything

@thompsongl
Copy link
Contributor

Right, I think I'd prefer avoiding the package update if we're planning on supplementing attributes anyway.

@thompsongl
Copy link
Contributor

Closing this in favor of post-render, dynamic attribute supplementation (see #2961)

@thompsongl thompsongl closed this Mar 9, 2020
@anishagg17 anishagg17 deleted the fix branch March 9, 2020 18:44
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.

[EuiCodeEditor] readonly mode is not accessible
5 participants