Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
004831e
[EuiInlineEdit] Add the isReadOnly prop that locks the component in r…
breehall May 16, 2023
8c08bc3
[EuiInlineEdit] Update snapshots to account for new inline_edit_form …
breehall May 16, 2023
0cdc03b
[EuiInlineEdit] Forgot to add snapshot updates
breehall May 16, 2023
8a7b117
[REVERT] Add documentation example to InlineEdit Text to display isRe…
breehall May 16, 2023
da47079
Revert "[REVERT] Add documentation example to InlineEdit Text to disp…
breehall May 16, 2023
e41cd89
[PR Review]
breehall May 16, 2023
f7cad78
[PR Feedback]
breehall May 16, 2023
fd36545
Update and add test cases
breehall May 16, 2023
cf3a1c9
[PR Feedback] - Update conditional aria-describedby span to surround …
breehall May 16, 2023
5dd01b0
Update src-docs/src/views/inline_edit/inline_edit_read_only.tsx
breehall May 17, 2023
c5930d8
Update src/components/inline_edit/inline_edit_form.styles.ts
breehall May 17, 2023
c46e6ed
[PR Feedback] -Add note in inline_edit_form.style.ts that we should r…
breehall May 17, 2023
531be08
[PR Feedback] Move the isReadOnly prop example up in the documentation
breehall May 17, 2023
13a0a02
Snapshots
breehall May 17, 2023
4375522
Update src-docs/src/views/inline_edit/inline_edit_example.js
breehall May 17, 2023
1940f35
[PR Feedback] - Removed conditiona lo logic to force a toggle from re…
breehall May 17, 2023
070d008
Merge branch 'inline-edit/readOnly' of https://github.com/breehall/eu…
breehall May 17, 2023
30c2e11
Oops!
breehall May 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src-docs/src/views/inline_edit/inline_edit_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,22 @@ const inlineEditModeSaveSnippet = `<EuiInlineEditText
import InlineEditValidation from './inline_edit_validation';
const inlineEditValidationSource = require('!!raw-loader!././inline_edit_validation');

import InlineEditReadOnly from './inline_edit_read_only';
const InlineEditReadOnlySource = require('!!raw-loader!././inline_edit_read_only');
const inlineEditReadOnlySnippet = `<EuiInlineEditText
inputAriaLabel="Edit text inline"
defaultValue="This is read only text!"
isReadOnly={isReadOnly}
/>

<EuiInlineEditTitle
inputAriaLabel="Edit title inline"
defaultValue="This is a read only title!"
heading="h3"
isReadOnly={isReadOnly}
/>
`;
Comment thread
breehall marked this conversation as resolved.
Outdated

export const InlineEditExample = {
title: 'Inline edit',
intro: (
Expand Down Expand Up @@ -203,7 +219,7 @@ export const InlineEditExample = {
properties
</li>
<li>
<EuiCode>editMode.inputRowProps</EuiCode> accepts any{' '}
<EuiCode>editMode.inputProps</EuiCode> accepts any{' '}
<Link to="/forms/form-controls#text-field">
<strong>EuiFieldText</strong>
</Link>{' '}
Expand Down Expand Up @@ -235,5 +251,25 @@ export const InlineEditExample = {
demo: <InlineEditModeProps />,
snippet: inlineEditModePropsSnippet,
},
{
title: 'Read only',
Comment thread
cee-chen marked this conversation as resolved.
Outdated
text: (
<>
<p>
Use the <EuiCode>isReadOnly</EuiCode> prop to lock{' '}
<EuiCode>EuiInlineEdit</EuiCode> in read mode and display the text
value. This does not affect the input form control in edit mode.
</p>
</>
),
source: [
{
type: GuideSectionTypes.TSX,
code: InlineEditReadOnlySource,
},
],
demo: <InlineEditReadOnly />,
snippet: inlineEditReadOnlySnippet,
},
],
};
39 changes: 39 additions & 0 deletions src-docs/src/views/inline_edit/inline_edit_read_only.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React, { useState } from 'react';

import {
EuiInlineEditText,
EuiInlineEditTitle,
EuiSpacer,
EuiSwitch,
} from '../../../../src';

export default () => {
const [isReadOnly, setIsReadOnly] = useState(true);

return (
<>
<EuiSwitch
label="Toggle isReadOnly"
Comment thread
breehall marked this conversation as resolved.
Outdated
checked={isReadOnly}
onChange={(e) => setIsReadOnly(e.target.checked)}
/>

<EuiSpacer />

<EuiInlineEditText
inputAriaLabel="Edit text inline"
defaultValue="This is read only text!"
isReadOnly={isReadOnly}
/>

<EuiSpacer />

<EuiInlineEditTitle
inputAriaLabel="Edit title inline"
defaultValue="This is a read only title!"
heading="h3"
isReadOnly={isReadOnly}
/>
</>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -643,13 +643,43 @@ exports[`EuiInlineEditForm Edit Mode renders 1`] = `
</div>
`;

exports[`EuiInlineEditForm Read Mode isReadOnly 1`] = `
<div
class="euiInlineEdit testClass1 testClass2"
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-tnjw7o-empty-disabled-euiInlineEditForm-isReadOnly"
data-test-subj="euiInlineReadModeButton"
disabled=""
type="button"
>
<span
class="euiButtonContent euiButtonContent--iconRight euiButtonEmpty__content"
>
<span
class="euiButtonEmpty__text"
>
Hello World!
</span>
</span>
</button>
<span
hidden=""
id="inlineEdit_generated-id"
>
Click this button to edit this text inline.
</span>
</div>
`;

exports[`EuiInlineEditForm Read Mode readModeProps 1`] = `
<div
class="euiInlineEdit testClass1 testClass2"
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-9t7nyf-empty-primary"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-6n5oyg-empty-primary-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -683,7 +713,7 @@ exports[`EuiInlineEditForm Read Mode renders 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -717,7 +747,7 @@ exports[`EuiInlineEditForm Read Mode sizes 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ exports[`EuiInlineEditText renders 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -44,7 +44,7 @@ exports[`EuiInlineEditText text sizes renders m 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -82,7 +82,7 @@ exports[`EuiInlineEditText text sizes renders s 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -120,7 +120,7 @@ exports[`EuiInlineEditText text sizes renders xs 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ exports[`EuiInlineEditTitle renders 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -44,7 +44,7 @@ exports[`EuiInlineEditTitle title sizes renders size l 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -82,7 +82,7 @@ exports[`EuiInlineEditTitle title sizes renders size m 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -120,7 +120,7 @@ exports[`EuiInlineEditTitle title sizes renders size s 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -158,7 +158,7 @@ exports[`EuiInlineEditTitle title sizes renders size xs 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -196,7 +196,7 @@ exports[`EuiInlineEditTitle title sizes renders size xxs 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down Expand Up @@ -234,7 +234,7 @@ exports[`EuiInlineEditTitle title sizes renders size xxxs 1`] = `
>
<button
aria-describedby="inlineEdit_generated-id"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-wvaqcf-empty-text"
class="euiButtonEmpty euiButtonEmpty--small euiButtonEmpty--flushBoth css-2otmvb-empty-text-euiInlineEditForm"
data-test-subj="euiInlineReadModeButton"
type="button"
>
Expand Down
25 changes: 25 additions & 0 deletions src/components/inline_edit/inline_edit_form.styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { css } from '@emotion/react';
import { UseEuiTheme } from '../../services';

export const euiInlineEditFormStyles = ({ euiTheme }: UseEuiTheme) => {
return {
euiInlineEditButton: css``,

// Override the cursor and allow users to highlight text when read mode is in the read only state
isReadOnly: css`
&.euiButtonEmpty:disabled {
Comment thread
cee-chen marked this conversation as resolved.
cursor: default;
Comment thread
breehall marked this conversation as resolved.
Outdated
color: ${euiTheme.colors.text};
user-select: text;
Comment thread
breehall marked this conversation as resolved.
}
`,
};
};
10 changes: 10 additions & 0 deletions src/components/inline_edit/inline_edit_form.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@ describe('EuiInlineEditForm', () => {
expect(container.firstChild).toMatchSnapshot();
});

test('isReadOnly', () => {
const { container, getByTestSubject } = render(
<EuiInlineEditForm isReadOnly={true} {...commonInlineEditFormProps} />
);

expect(container.firstChild).toMatchSnapshot();

expect(getByTestSubject('euiInlineReadModeButton')).toBeDisabled();
});

test('readModeProps', () => {
const { container, getByTestSubject } = render(
<EuiInlineEditForm
Expand Down
31 changes: 24 additions & 7 deletions src/components/inline_edit/inline_edit_form.tsx
Comment thread
cee-chen marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { EuiSkeletonLoading, EuiSkeletonRectangle } from '../skeleton';
import { useEuiTheme, useCombinedRefs, keys } from '../../services';
import { EuiI18n, useEuiI18n } from '../i18n';
import { useGeneratedHtmlId } from '../../services/accessibility';
import { euiInlineEditFormStyles } from './inline_edit_form.styles';

// Props shared between the internal form component as well as consumer-facing components
export type EuiInlineEditCommonProps = HTMLAttributes<HTMLDivElement> &
Expand Down Expand Up @@ -80,6 +81,10 @@ export type EuiInlineEditCommonProps = HTMLAttributes<HTMLDivElement> &
* Invalid state - only displayed edit mode
*/
isInvalid?: boolean;
/**
* Locks inline edit in read mode and displays the text value
*/
isReadOnly?: boolean;
};

// Internal-only props, passed by the consumer-facing components
Expand Down Expand Up @@ -122,10 +127,18 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
isLoading = false,
isInvalid,
onSave,
isReadOnly,
}) => {
const classes = classNames('euiInlineEdit', className);

const euiTheme = useEuiTheme();

const styles = euiInlineEditFormStyles(euiTheme);
const readOnlyStyles = [
styles.euiInlineEditButton,
isReadOnly && styles.isReadOnly,
];

const { controlHeight, controlCompressedHeight } = euiFormVariables(euiTheme);
const loadingSkeletonSize = sizes.compressed
? controlCompressedHeight
Expand Down Expand Up @@ -228,12 +241,14 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
)}
/>
</EuiFormRow>
<span id={editModeDescribedById} hidden>
<EuiI18n
token="euiInlineEditForm.inputKeyboardInstructions"
default="Press Enter to save your edited text. Press Escape to cancel your edit."
/>
</span>
{!isReadOnly && (
<span id={editModeDescribedById} hidden>
<EuiI18n
token="euiInlineEditForm.inputKeyboardInstructions"
default="Press Enter to save your edited text. Press Escape to cancel your edit."
/>
</span>
)}
Comment thread
cee-chen marked this conversation as resolved.
Outdated
</EuiFlexItem>

<EuiFlexItem grow={false}>
Expand Down Expand Up @@ -296,12 +311,14 @@ export const EuiInlineEditForm: FunctionComponent<EuiInlineEditFormProps> = ({
<>
<EuiButtonEmpty
color="text"
iconType="pencil"
iconType={isReadOnly ? undefined : 'pencil'}
iconSide="right"
flush="both"
iconSize={sizes.iconSize}
size={sizes.buttonSize}
data-test-subj="euiInlineReadModeButton"
disabled={isReadOnly}

@cee-chen cee-chen May 16, 2023

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.

@1Copenut I'd super appreciate your thoughts on this, I'm not totally convinced just the disabled attribute here is sufficient. VO still reads out "dimmed button" for me.

I could be wrong, but I think we want the semantic meaning of the text or title to revert to how it would be when it's not in a button, which in this case means adding manual roles:

EuiText example:

<button disabled role="paragraph">Some non editable text</button>

EuiTitle example:

<button disabled role="heading" aria-level="3"><h3>Some non-editable title</h3></button>

Thoughts? Honestly, if I could I'd get rid of the button wrapper entirely, but I don't think there's an easy way to accomplish that and still retain the correct styles / sizing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cee-chen

Honestly, if I could I'd get rid of the button wrapper entirely, but I don't think there's an easy way to accomplish that and still retain the correct styles / sizing

This was honestly the first thing I tried, but it was no easy feat. The There's a mismatch in line-height and text styling that isn't so easy to replicate outside of the button. It doesn't help that EuiButtonEmpty and its friends aren't all converted to Emotion yet 😅

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.

Haha woof. Yeah, I totally get that. Unfortunately role workarounds may be the best option for now in that case.

You can set those props conditionally from the parent EuiInlineEditText/Title components by creating a new version of readModeProps with role/aria-level conditionally set if isReadOnly is true, and passing that down to EuiInlineEditForm.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just update this branch with a potential solution for this. I'll wait to hear what y'all think!

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 you're onto something here. I'm experimenting with ways to get the disabled buttons to stop announcing as "dimmed" but haven't hit on the right combination yet. I'll update tomorrow if I find something more useful.

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.

Sadly on VO, I couldn't get passed the dimmed announcement at all without removing disabled (which as other bad implications for focus, click, etc).

We could maybe add an aria-describedby text that explains the dimmed as "this is normally an editable button, but is currently set to read only mode"... but that doesn't feel great / feels overly verbose. 😬

if we can't get away from using EuiEmptyButton/button, I think we have to live with the "dimmed" verbiage 😬

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.

@1Copenut separately, I'm curious what you think about potentially adding the role="heading" and aria-level prop to EuiInlineEditTitle regardless of whether it's readonly or not - despite the button having a <h3> heading in it, that level doesn't get read out for me in VO+FF (not sure about other SRs). Is the heading at least still available as a screen reader page landmark?

@1Copenut 1Copenut May 18, 2023

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.

Cee, you read my mind. I was experimenting with adding a conditional role to the <H> component based on isReadOnly that caused VO to start reading the heading level correctly. We wouldn't need the aria-level because that could cause the heading level to be read twice. I'm thinking something like the following (in a separate PR):

// src/components/inline_edit/inline_edit_title.tsx#L99

<H role={isReadOnly ? 'presentation' : 'heading'}>{titleReadModeValue}</H>

I'm going from memory what I was noodling with last night, so it might take a couple of experiments, but I did find a way to get the heading announcing in Safari + VO, so hopefully Firefox + VO follows suit. NVDA did a better job announcing the heading and treating it as a heading "stop" in Chrome and Firefox.

css={readOnlyStyles}
{...readModeProps}
buttonRef={setReadModeRefs}
aria-describedby={classNames(
Comment thread
cee-chen marked this conversation as resolved.

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.

The readOnly heading is still announcing the aria-describedby text, unsure as to root cause. I tried out a conditional block and it seemed to work pretty well. Can't put it in the right place but basically this is my proposed change:

! src/components/inline_edit/inline_edit_form.tsx#343

{!isReadOnly && (
+ <>
    <span id={readModeDescribedById} hidden>
      <EuiI18n
        token="euiInlineEditForm.activateEditModeDescription"
        default="Click this button to edit this text inline."
      />
    </span>
+ </>
)}

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.

@1Copenut that's super bizarre - is NVDA caching the text somehow?

Won't axe throw an error since the aria-describedby is still pointing to an ID that no longer exists? Do we need to modify aria-describedby as well?

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.

Cee is right. We'll need to unset the aria-describedby attribute and the EuiScreenReaderOnly component when the component is in readOnly state.

Expand Down
Loading