[EuiMarkdownEditor] Add `readOnly` prop by elizabetdev · Pull Request #5627 · elastic/eui

elizabetdev · 2022-02-10T22:28:44Z

Summary

Closes #5129. This PR adds a readOnly prop to EuiMarkdownEditor.

Initially, I started this PR by adding an isDisabled prop. But after some discussion, I decided to add a readOnly prop instead. The reasoning for this decision can be found in the following comment: #5627 (comment).

Design

Checklist

Check against all themes for compatibility in both light and dark modes
[ ] Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added documentation
~~[ ] Checked Code Sandbox works for any docs examples~~
Added or updated jest and cypress 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 · 2022-02-10T23:00:27Z

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

…abled-state

kibanamachine · 2022-02-11T15:59:35Z

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

chandlerprall · 2022-02-14T20:57:10Z

@miukimiu I pushed a small change that makes the checkbox plugin respect your isDisabled context

kibanamachine · 2022-02-14T21:39:01Z

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

…abled-state

elizabetdev · 2022-02-15T14:06:58Z

Thanks, @chandlerprall for helping with the issue with the checkboxes not getting disabled.

I have another question. How should we handle the isDisabled state for custom plugins? For example, in our custom plugin (https://eui.elastic.co/pr_5627/#/editors-syntax/markdown-plugins), the consumer application could use CSS and apply an opacity by targeting .euiMarkdownEditorPreview-isDisabled .myCustomChartPlugin

But we can have more complex plugins that the consumer would need to get the state because not always just CSS would work. Like in the checkboxes plugin for example where we got the isDisabled state from EuiMarkdownContext.

When creating plugins is this state exposed so that consumers can get it? Do we need to expose it? And how do we document this?

kibanamachine · 2022-02-15T14:07:52Z

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

cee-chen · 2022-02-15T16:31:55Z

I haven't looked super closely at this PR at all but just wanted to suggest something potentially very dumb.

Why not cover the entire editable contentarea of the editor with an overlay div of the same background color as the editor and a 50% opacity? This serves the following:

No need to set manually set any child plugins to any kind of opacity. The overlay takes care of it (edit: would also solve the linked emoji problem)
Blocks mouse clicks entirely, no need to dive into every single child to make sure they are also disabled or uninteractive

This may need some extra finagling to ensure that the underlying editor is inaccessible to keyboard/screen reader users, but for visual and mouse users it should work 🤷‍♀️

EDIT: If an overlay div is too basic we could also look into CSS filters

…abled-state

kibanamachine · 2022-02-16T14:28:03Z

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

elizabetdev · 2022-02-16T17:23:01Z

Thanks, @constancecchen for suggesting the overlay div.

I changed the PR back to draft. Based on our weekly sync discussion I decided to add a readOnly prop to the EuiMarkdownEditor instead of an isDisabled prop.

For our use case "adding a comment", we just want to restrict editing but users should be able to read properly what they wrote once they post the comment.

If we decided to add an isDisabled prop the styles would make the text grayed out. This would make users not see properly what they are submitting.

But how do we notice that editing is restricted?

In editing mode, all toolbar and footer buttons get disabled. This would make it more visible to users that the editor is in a different state and no more editing is possible. The textarea gets a readonly attribute. Visually the textarea looks the same, but the content cannot be changed.

In previewing mode the toolbar buttons are already disabled. The only difference from a normal state is the editor mode button gets also disabled. All the rest look the same.

Components that allow changing the content like checkboxes should not be clickable. Plugins that can modify the content, should implement their logic to respect the readOnly state. @chandlerprall is going to expose this readOnly state to use in plugins.

This can be tested in https://eui.elastic.co/pr_5627/#/editors-syntax/markdown-editor. The logic for the checkboxes plugin in previewing mode is not implemented yet. For now, they just get disabled.

Any thoughts on this new approach?

elizabetdev · 2022-02-16T17:34:58Z

src/components/markdown_editor/_markdown_editor_text_area.scss

 }
+
+.euiMarkdownEditorTextArea-isReadOnly {
+  cursor: default;


This cursor: default is following our EuiTextArea styles. But I'm not quite sure is really necessary... If we can copy the text shouldn't we keep the cursor that a textarea gets by default cursor: text;?

When hovering on an enable textarea, the cursor everywhere is text indicating it is enabled and to click anywhere to insert text. The default cursor overrides this behavior to try to indicate that you can no longer edit by showing the arrow cursor everywhere. What we probably want to do instead is cursor: unset; which defaults back to the browser's behavior of arrow cursor until hovering over text. I checked this specifically on our EuiTextArea and it works the way I'd expect, but I didn't check the Markdown editor.

The EuiTextArea and other form controls when in readOnly get a cursor: default;:

eui/src/global_styling/mixins/_form.scss

Lines 216 to 222 in 6b3279d

@mixin euiFormControlReadOnlyStyle {

cursor: default;

// Use transparency since there is no border and in case form is on a non-white background

background: $euiFormBackgroundReadOnlyColor;

border-color: transparent;

box-shadow: inset 0 0 0 1px $euiFormBorderDisabledColor;

}

But almost every browser behavior when in readOnly gets a cursor: text;. This cursor indicates that the text can be selected (https://developer.mozilla.org/en-US/docs/Web/CSS/cursor).

So I'm just adding this cursor: default; because I'm following the pattern of our EuiTextArea. Is this the behavior we want to maintain?

Right so, we probably want to align better with browser's innate use of cursor. If we're using the HTLM property readonly which applies the pseudo class :read-only we shouldn't have to change the cursor. You can undo our non-readonly cursor by just reverting it back to the browser's default via cursor: unset. But I can't be positive that all uses of the mixin is in fact applying the readonly property directly.

If we can copy the test shouldn't we keep the cursor that a textarea gets by default cursor: text;?

Agreed. It feels weird with this set to default

chandlerprall · 2022-02-16T19:21:33Z

Changed the replaceNode and openPluginEditor functions, which are given to plugins, to do nothing when in read only mode.

I played with the checkbox styles some but didn't like anything, and I couldn't find an established pattern anywhere around what a readonly checkbox should look like - best I came up with was cursor: default to remove the visual indication that it's clickable.

kibanamachine · 2022-02-16T20:05:27Z

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

…abled-state

kibanamachine · 2022-02-17T13:40:19Z

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

…abled-state

kibanamachine · 2022-02-22T17:27:35Z

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

i-a-n

just wanted to say from a UI/UX standpoint I really like it, and it solves our use-case perfectly 👍

cchaos

Visually & functionally everything works great. Had a couple code & docs suggestions.

cchaos · 2022-02-22T20:17:31Z

src-docs/src/views/markdown_editor/markdown_editor.js

+        <EuiFlexItem grow={true}>
+          <EuiSwitch
+            label="Read-only"
+            checked={isReadOnly}
+            onChange={onChange}
+          />
+        </EuiFlexItem>


I highly suggest adding a sentence about the use of this prop to the text of this example. Maybe explaining, that if the consumer needs to disallow editing while submission is happening, then they should manually update this prop.

What do you think of this text?

I like it! I just have a quick suggestion to tighten up the language.

src/components/markdown_editor/_markdown_editor_text_area.scss

src/components/markdown_editor/markdown_editor_text_area.tsx

src/components/markdown_editor/markdown_editor_toolbar.tsx

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

…abled-state

kibanamachine · 2022-02-23T14:20:42Z

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

cchaos

LGTM!

cchaos · 2022-02-23T15:00:08Z

src-docs/src/views/markdown_editor/markdown_editor.js

+        <EuiFlexItem grow={true}>
+          <EuiSwitch
+            label="Read-only"
+            checked={isReadOnly}
+            onChange={onChange}
+          />
+        </EuiFlexItem>


I like it! I just have a quick suggestion to tighten up the language.

src-docs/src/views/markdown_editor/mardown_editor_example.js

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2022-02-23T16:01:56Z

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

Added isDisabled prop to EuiMarkdownEditor

bf09b65

elizabetdev added 2 commits February 11, 2022 15:11

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

7c14d7c

…abled-state

Adding in CL correct PR number

6b623c0

Make markdown checkboxes respect isDisabled state

1982f97

elizabetdev added 4 commits February 15, 2022 11:59

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

68f589b

…abled-state

Adding all plugins into the base editor example.

e1c1bca

Removing pointer events none

1a2bead

Preventing components like the tooltip to work in disabled state

7dbe54d

elizabetdev mentioned this pull request Feb 15, 2022

[EuiMarkdownEditor] Emojis a11y and disabled states #5637

Closed

elizabetdev marked this pull request as ready for review February 15, 2022 13:47

elizabetdev added 2 commits February 16, 2022 12:49

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

23d70ed

…abled-state

Adding readOnly state

2b4b314

elizabetdev marked this pull request as draft February 16, 2022 16:28

elizabetdev changed the title ~~[EuiMarkdownEditor] Add isDisabled prop~~ [EuiMarkdownEditor] Add readOnly prop Feb 16, 2022

elizabetdev commented Feb 16, 2022

View reviewed changes

prevent markdown changes in readonly mode

9cf3408

Checkboxes -isReadOnly styles

5bf9372

elizabetdev requested a review from i-a-n February 17, 2022 12:54

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

4025a6d

…abled-state

elizabetdev marked this pull request as ready for review February 17, 2022 13:07

elizabetdev requested review from cchaos and chandlerprall February 17, 2022 16:58

elizabetdev added 2 commits February 22, 2022 16:49

Reverting euiFormControlReadOnlyStyle mixin

53478ee

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

1cd7fde

…abled-state

i-a-n approved these changes Feb 22, 2022

View reviewed changes

cchaos reviewed Feb 22, 2022

View reviewed changes

elizabetdev and others added 5 commits February 23, 2022 11:56

Update src/components/markdown_editor/markdown_editor_toolbar.tsx

0bc46e6

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Addressing PR review

d0b553b

Merge remote-tracking branch 'upstream/main' into markdown-editor-dis…

5b2d50a

…abled-state

Removing unnecessary type

cd2e9c6

Text

f9312e0

cchaos approved these changes Feb 23, 2022

View reviewed changes

elizabetdev and others added 2 commits February 23, 2022 15:17

Update src-docs/src/views/markdown_editor/mardown_editor_example.js

b73a006

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

Making prettier!

575a6fa

elizabetdev merged commit 4e5ac45 into elastic:main Feb 23, 2022

cee-chen mentioned this pull request Feb 23, 2022

[EuiSuggest][EuiSelectable] Allow controlled search value prop #5658

Merged

8 tasks

thompsongl mentioned this pull request Mar 4, 2022

Upgrade EUI to v51.1.0 elastic/kibana#126926

Merged

	@mixin euiFormControlReadOnlyStyle {
	cursor: default;
	// Use transparency since there is no border and in case form is on a non-white background
	background: $euiFormBackgroundReadOnlyColor;
	border-color: transparent;
	box-shadow: inset 0 0 0 1px $euiFormBorderDisabledColor;
	}

Conversation

elizabetdev commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design

Checklist

Uh oh!

kibanamachine commented Feb 10, 2022

Uh oh!

kibanamachine commented Feb 11, 2022

Uh oh!

chandlerprall commented Feb 14, 2022

Uh oh!

kibanamachine commented Feb 14, 2022

Uh oh!

elizabetdev commented Feb 15, 2022

Uh oh!

kibanamachine commented Feb 15, 2022

Uh oh!

cee-chen commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Feb 16, 2022

Uh oh!

elizabetdev commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elizabetdev Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chandlerprall commented Feb 16, 2022

Uh oh!

kibanamachine commented Feb 16, 2022

Uh oh!

kibanamachine commented Feb 17, 2022

Uh oh!

kibanamachine commented Feb 22, 2022

Uh oh!

i-a-n left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Feb 23, 2022

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

elizabetdev commented Feb 10, 2022 •

edited

Loading

cee-chen commented Feb 15, 2022 •

edited

Loading

elizabetdev commented Feb 16, 2022 •

edited

Loading

elizabetdev Feb 16, 2022 •

edited

Loading