Skip to content

Conversation

@breehall
Copy link
Contributor

@breehall breehall commented May 16, 2023

Included in #3928

Summary

✅ Create the isReadOnly prop for EuiInlineEdit. When the prop is true, a disabled EuiEmptyButton is displayed which locks the user in read mode and displays the text value. This does not affect edit mode.

Screen.Recording.2023-05-16.at.9.50.49.AM.mov

QA

  • View the new Inline Edit Read Only example in the PR preview to toggle the isReadOnly prop. When the prop is set to true, you should not be able to transfer to edit mode. You should also see the default cursor and should be able to highlight and copy text.

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added documentation
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes

…ead mode and does not allow the user to update text
@breehall breehall added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label May 16, 2023
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@breehall breehall marked this pull request as ready for review May 16, 2023 14:19
@breehall breehall requested review from 1Copenut and cee-chen May 16, 2023 14:19
iconSize={sizes.iconSize}
size={sizes.buttonSize}
data-test-subj="euiInlineReadModeButton"
disabled={isReadOnly}
Copy link
Contributor

@cee-chen cee-chen May 16, 2023

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
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
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
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
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
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
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?

Copy link
Contributor

@1Copenut 1Copenut May 18, 2023

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.

breehall added 4 commits May 16, 2023 15:45
…lay isReadOnly prop"

This reverts commit 8a7b117.

Revert change made to demo for testing
- Add an official example for the isReadOnly prop
- Name correction in edit mode props example
- Updated the name of Emotion style object to indicate we're styling the read mode button, not the entire form
- Conditionally hide the text content of aria-describedby when isReadOnly is true
- Conditionally add a role (and aria-level where required) to the read mode button when isReadOnly is true
@breehall breehall requested a review from cee-chen May 16, 2023 20:27
@kibanamachine
Copy link

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

…the text instead of the entire span.

- Clean up logic related to adding aria-live and role attributes when isReadOnly is true
@kibanamachine
Copy link

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

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

This is looking really good Bree! I have 1 or 2 change requests and a couple very optional comments. If @1Copenut is busy with other things today I think we could merge this PR in as-is (after the requested changes) and do a final a11y/screen reader pass on the main feature branch later.

Comment on lines +53 to +56
expect(getByTestSubject('euiInlineReadModeButton')).toHaveAttribute(
'aria-level',
'1'
);
Copy link
Contributor

Choose a reason for hiding this comment

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

[Super optional, feel free to skip] You could use RTL's getByRole API here to make this a bit shorter code-wise!

const { container, getByTestSubject, getByRole } = render(...);

expect(getByRole('heading')).toHaveAttribute('aria-level', '1');

breehall and others added 4 commits May 17, 2023 14:43
…eevaluate styles once EuiEmptyButton is converted to Eotion.

- Updated render condition for edit mode to ensure if isReadOnly is true, we stay / kick back to read mode
- Test case & snapshot updates
@kibanamachine
Copy link

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

@breehall breehall requested a review from cee-chen May 17, 2023 19:14
@breehall
Copy link
Contributor Author

Sorry to ping your for review again, Cee! I appreciate your help with this!

@kibanamachine
Copy link

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

breehall added 2 commits May 17, 2023 16:01
…ad mode to edit mode when isReadOnly is true. Opted for a useEffect that will handle the change instead
…into inline-edit/readOnly

Merge commits made in Github UI
@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

@breehall I've got one change so far. I'll pick up tomorrow and experiment more with NVDA and see how it drives. I'll update my review if I find anything else that could be adjusted.

css={readOnlyStyles}
{...readModeProps}
buttonRef={setReadModeRefs}
aria-describedby={classNames(
Copy link
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
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
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.

iconSize={sizes.iconSize}
size={sizes.buttonSize}
data-test-subj="euiInlineReadModeButton"
disabled={isReadOnly}
Copy link
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
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

This LGTM from a UX/code perspective.

Re: screen reader shenanigans, @1Copenut would it make more sense to have that be a separate PR where we address various SR issues, since you already have a separate set of comments on the main feature branch?

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I left 2 comments for screen reader improvements that would be well-served as a new PR. Thank you to @cee-chen for suggesting this!

@breehall
Copy link
Contributor Author

breehall commented May 18, 2023

Next level accessibility enhancements

  • Unset the aria-describedby attribute and the EuiScreenReaderOnly component when the component is in readOnly state. (ref)
  • Update EuiInlineEditTitle header elements to always have a role. When isReadOnly is false, the role should be presentation(for both heading and span elements). We also need to remove the aria-level attribute if isReadOnly is true as it could cause the heading to be read twice. (ref)
  • See if there's a way to make Safari + VO honor the Enter keypress to save

@ 1Copenut Is this an accurate description of what the next PR should handle?

@1Copenut
Copy link
Contributor

1Copenut commented May 18, 2023

Next level accessibility enhancements

  • Unset the aria-describedby attribute and the EuiScreenReaderOnly component when the component is in readOnly state. (ref)
  • Update EuiInlineEditTitle header elements to always have a role. When isReadOnly is false, the role should be presentation(for both heading and span elements). We also need to remove the aria-level attribute as it could cause the heading to be read twice. (ref)

@1Copenut Is this an accurate description of what the next PR should handle?

@breehall Yes, these are the items I'd recommend changing. Could we revise the second point and add one more?

  • Don't remove aria-level from the button[role="heading"] in read only mode. The combination of role and aria-level "make" the button a heading. Add the role isReadOnly ? 'presentation' : 'heading' to the <H> tag .
  • If there's an easy fix to make Safari + VO honor the Enter keypress to save, that'd be excellent.

@breehall
Copy link
Contributor Author

Absolutely! Thank you for the clarification. I just updated the checklist so these next three items with be the target of the next PR. I'll go ahead and merge this one and get started on the next! Thank you all!

@breehall breehall merged commit a864a9f into elastic:feature/EuiInlineEdit May 18, 2023
@cee-chen
Copy link
Contributor

See if there's a way to make Safari + VO honor the Enter keypress to save

Fingers crossed, but I'm hoping that might be as easy as e.preventDefault() (which I incorrectly claimed we didn't need earlier 😅). There appears to be some amount of shenanigans under the hood on Enter for inputs. It's really interesting to me that there isn't that behavior for Escape

cee-chen added a commit that referenced this pull request May 25, 2023
…onents (#6757)

* [EuiInlineEdit] Create Component Directory and Base Functionality (#6598)

* Initial directory setup for EuiInlineEdit

* Initial structure for EuiInlineEdit docs

* Created EuiInlineEdit base component that toggles between a text input and text or title.

* Replaced button icons with full EuiButtons with text. Positioned the buttons on the right side of the input

* Added early snapshot for component

* Added aria-labels with i18n for the editView input, save button, and cancel buttons. Updated the props to require an input aria label because the option to show the label on the input was removed. This was removed to preserve the layout and prevent the input from appearing to bounch when switching between editView and readView.

* Combined with last commit - forgot to hit save

* Made defaultValue a required field because if there is no value passed in, the button will be empty (with the exception of the edit icon)

* Updated basic snapshot tests, updated docs

* Added button groups to documentation to make toggling button sizes easier

* Update src/components/text/index.ts

Co-authored-by: Cee Chen <[email protected]>

* Update src-docs/src/views/inline_edit/inline_edit.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src-docs/src/views/inline_edit/inline_edit.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit.test.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit.tsx

Co-authored-by: Elizabet Oliveira <[email protected]>

* Separated the two EuiInlineEdit examples into their own files

* Created additional logic to resize buttons and fieldtext elements when when the size of EuiInlineEdit is small or lower.

* Created a heading prop to allow consumers to choose the level heading they would like the InlineEdit (title) to be wrapped inside of. If one isn't provided, h2 will be the default.

* Updated basic test snapshots for EuiInlineEdit

* Updated new i18n tokens to be more explicit about their purpose

* Split EuiInlineEdit into three components: EuiInlineEditText, EuiInlineEditTitle, and EuiInlineEditButtons.
- Created utils and types file to share common props and utility functions between all three components

* Oops

* EuiInlineEdit Updates
- Updated the onConfirm prop to return a boolean flag that will determine if the text value is saved or not in editMode
- Updated edit/read prop variable names
- Exported EuiButtonEmptyPropsForButton because it's being used as a type for the new readMode prop
- Added base testing files for EuiInlineEditTitle and EuiInlineEditButtons

* Remove testing props from inline edit example

* Separated repeated logic inside of EuiInlineEditTitle and EuiInlineEditText into a new component called EuiInlineEditForm. This new form component is responsible for the toggling of readMode and editMode for EuiInlineEdit.

* Move types to shared internal form component

* Pass top-level props instead of in a `props` obj

* Remove top level read state, use render prop instead

- so that EuiInlineEditForm is completely in charge of state

* Remove EuiInlineEditButtons, roll into EuiInlineEditForm as inline

* Removed utility in favor of component-specific size logic

- while still DRYing out the underlying form sizes being used

* Fix types and update snapshots

* Clean up type def

---------

Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Elizabet Oliveira <[email protected]>
Co-authored-by: Constance Chen <[email protected]>

* [EuiInlineEdit] Add Dynamic Font Sizing & Truncation Styling (#6660)

* Created styling for EuiInlineEdit text and title components that adjusts the font-size used within the editMode form control. Added styling to ensure that very long text is truncated inside of the EuiEmptyButton used in readMode

* Updated snapshot tests for EuiInlineEdit

* Update src/components/inline_edit/inline_edit_title.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit_text.tsx

Co-authored-by: Cee Chen <[email protected]>

* Updated snapshots for EuiInlineEdit test cases after modification in Github UI

---------

Co-authored-by: Cee Chen <[email protected]>

* [EuiInlineEdit] Creation of the `isLoading` and `isInvalid` Props (#6667)

* Added isInvalid and isLoading props to EuiInlineEdit components (form, title, text).

* [REVERT ME] Added prop toggles to the EuiInlineEditText example for validation

* Revert "[REVERT ME] Added prop toggles to the EuiInlineEditText example for validation"

This reverts commit b72c9ca.

Reverting documentation change that was for PR assistance. It's no longer needed.

* Updated EuiInlineEditForm by removing the isLoading prop on the confirm and cancel buttons. Instead, these are wrapped in the skeleton component when loading.

* Added a new documentation view for the isLoading prop

* Updated the editModeProps property to include the erorr prop from EuiFormRowProps. Additionally wrapped EuiFieldText inside of EuiFormRow. This is to allow consumers to pass an error message to the EuiFormRow when the isValid prop is false.

* -[PR Feedback] Removed the onChange method from the loading example and placed it directly on the switch element
- Created the isValid documentation example

* Remove blunder

* Added the skeletonHeight index to sizes object. The Skeleton buttons used when isLoading is true need to be smaller when the form is compressed

* [PR Feedback - Update the editModeProps prop to include two sub-objects. This includes one that will be placed on the form row and the other will be placed on the input form control in edit mode

* [PR Feedback - Combined the isLoading and isValid prop documentation examples. Removed the isValid example completely.

* [PR Feedback - Remove hard coded values used for the EuiSkeletonRectangle height when isLoading is true

* Update src/components/inline_edit/inline_edit_form.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit_form.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit_form.tsx

Co-authored-by: Cee Chen <[email protected]>

* Updated docs that used the editModeProps prop with EuiInlineEdit

* Removed unused code block in EuiInlineEditForm

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Cee Chen <[email protected]>

* [PR Feedback] - Removed readModeProps from InlineEditTitle example. Allowed Prettier to format the copy on a documentation file

---------

Co-authored-by: Cee Chen <[email protected]>

* [EuiInlineEdit] Create `EuiInlineEdit` Docs (#6697)

* Export the EuiInlineEdit Text and Title prop types

* Created docs examples for EuiInlineEdit props

* Removed an export for EuiInlineEditTextSizes as devs can drill into Title and Text props for the size

* [PR Feedback] - Reorder sections of the EuiInlineEdit docs. Update the copy included in a few examples for better understanding

* [Oops] Removed and replaced the EuiInlineEditTextSize import in InlineEdit Text example

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Cee Chen <[email protected]>

---------

Co-authored-by: Cee Chen <[email protected]>

* [EuiInlineEdit] Test Cases for `EuiInlineEdit` (#6722)

* Added Cypress tests for EuiInlineEdit functionality

* Added snapshots for read mode and edit mode for the Text and Title variations for EuiInlineEdit

* Use RTL render method for snapshots

* [InlineEditForm] - Add data-test-subj to EuiInlineEditForm buttons and inputs
- Removed opinionated save logic that prevented users from being able to save empty strings in editMode

* Created test cases for EuiInlineEditForm. These test cases handle the functional and prop testing for both the Text and Title variations of the component

* Updated InlineEdit Text and Title test suites to only include basic render tests and snapshots of their sizing in both readMode and editMode

* Removed Cypress test in favor of testing EuiInlineEdit with Jest

* Created the onSave prop that returns that latest value within EuiFieldText (in editMode) at the time the save button is clicked.

* [REVERT ME] Added text to the InlineEditText example to display the use of the onSave prop

* Updated onSave prop test to use the Jest mock function call instead of relying on variable value changes

* [PR Feedback] Remove tests that toggled between read/edit mode in EuiInlineEdit Text and Title test suites.

* [PR Feedback] - Updated the descriptions for the onSave and onConfirm props
- Removed data-test-subj IDs from the loading skeleton rectangles
- Updated the isLoading prop to default to false and removed explicit declarations in Text and Title

* [PR Feedback] Updated EuiInlineEditForm test cases with more specific cases for onSave. Removed the isLoading default set in the props for the form as it's no longer needed

* Revert "[REVERT ME] Added text to the InlineEditText example to display the use of the onSave prop"

This reverts commit 6005e53.

Removing modifications to docs file as they were only needed for testing

* Created a new documentation example to showcase the onSave prop

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Cee Chen <[email protected]>

---------

Co-authored-by: Cee Chen <[email protected]>

* [EuiInlineEdit] Refactor multiple props to harden for real-world production usage + misc cleanup (#6735)

* Save example misc cleanup

- remove button that clears localStorage - it's not necessary for the demo and we can do so in devtools instead

- remove onConfirm doc copy - going to move that to a new section

* Add support for async saving + validation

- remove confusing extra `onConfirm` callback for handling everything in a single `onSave`

- remove extra `onSave` prop test - just roll that behavior into the last describe block

* Update documentation examples

- update confirm example to more closely mimic production behavior

- remove dedicated isLoading/isInvalid demo - we can demo that in the production example instead

* Remove specific `ariaLabel` props in favor of generic props that can be spread to the save & cancel buttons

- this is necessary for consumers to do things like reset errors on cancel, or fire telemetry events on click

+ remove `disabled` state of save button on `isInvalid` - but consumers can add that back in if needed via `saveButtonProps`, depending on their specific UX

+ minor type cleanup

* Support custom `onClick` events for all button props

- clicks should call both internal EUI handlers and consumer callbacks

* Misc cleanup

- Remove unnecessary toggle on isEditing switches which could potentially create race conditions - we know it should always be either true or false depending on the mode being entered/exited

- Improve prop docs readability

- make `it` vs. `test` syntax/grammar more consistent

- Remove unnecessary false fallbacks - undefined is already falsy

* Improve docs a bit more

- react typegen is mutilating `editModeProps`, so we should more explicit in our manual prop docs

- update the last docs example

* PR feedback: typo

Co-authored-by: Bree Hall <[email protected]>

---------

Co-authored-by: Bree Hall <[email protected]>

* [EuiInlineEdit] Add `onKeyDown` Mapping for Saving and Canceling Edits (#6742)

* [EuiInlineEdit] Add keydown events that map the Enter and Escape keys to the save and cancel functions respectively.

* [REVERT ME] Added a custom keydown event that overrides our Enter mapping as an example

* [PR Feedback] Various structural updates including renaming test cases and updating logic to run both the default and custom onKeyDown methods for the edit mode input.

* [PR Feedback] Remove autoFocus from both the read mode button and edit mode input in favor of manual focus management. Created a ref/useEffect combo that keeps track of the state and sets focus on the edit mode input only when the read mode button is clicked.

* Add aria-describedby property to the edit mode input

* Revert commit c48ee4f.

* Revert 2460856

* Pull aria-describedby outside of the edit mode input form row

* Update test case to validate that custom onKeyDown events work along side the default events

* [EuiInlineEdit] Add Playground & Code Snippets (#6743)

* [EuiInlineEdit] Create a playground for both EuiInlineEdit Text and Title

* [EuiInlineEdit] Add code snippets for each InlineEdit example in the docs

* Removed the inline edit validation code snippet in favor of the DemoJS because it's more helpful

* Small updates to DRY util.

* Added the span option to the list of headings available for EuiInlineEdit Title.

* Update EuiInlineEdit Title heading prop documentation for clarity

* Update src/components/inline_edit/inline_edit_title.tsx

Co-authored-by: Cee Chen <[email protected]>

---------

Co-authored-by: Cee Chen <[email protected]>

* [EuiInlineEdit] DOM cleanup, a11y improvements, and fix breaking `onChange` bug (#6746)

* DOM cleanup

- Remove unnecessary `EuiForm` - we're not taking advantage of form submit behavior, so we might as well remove this

- Remove unnecessary `EuiFormRow`s around save/cancel buttons - they're not inputs and don't need form rows

- Remove incorrect `className={classes}` placement

* Use `EuiSkeletonLoading` instead of 2 `EuiSkeletonRectangle`s

- this prevents double loading screen reader announcements

* Update EuiInlineEdit to only announce when state flips to loading

- so that a "loaded" announcement doesn't occur whenever edit mode is opened, which doesn't make a whole lot of sense

* Improve SR UX of read mode

- add an `aria-describedby` that explains to screen reader users what happens when the read mode button is clicked, since the text itself is not indicative

+ cleanup: remove unused input ID

* Fix `editModeProps.inputProps.onChange` overriding/breaking behavior

* Remove `autoFocus` in favor of imperative `.focus()` calls on click

- rAF is needed to wait a tick after state has changed and DOM node is available

+ write tests (with waitFor) asserting that focus state works as expected (mocking rAF primarily for test/function coverage)

* [PR feedback] `toggle`->`activate`

* Removed unneeded style file and references to it. (#6751)

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Trevor Pierce <[email protected]>

* Update src/components/inline_edit/inline_edit_form.tsx

Co-authored-by: Trevor Pierce <[email protected]>

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Trevor Pierce <[email protected]>

* [EuiInlineEdit] `CHANGELOG` & Copy Linting (#6788)

* Changelog & text linting from changes made in Github UI

* Snapshot update as a result of updating the copy of the aria-describedby used by inline edit

* [EuiInlineEdit] Create `isReadOnly` Prop for Read Mode (#6777)

* [EuiInlineEdit] Add the isReadOnly prop that locks the component in read mode and does not allow the user to update text

* [EuiInlineEdit] Update snapshots to account for new inline_edit_form styles. Added a test case for the new isReadOnly prop

* [EuiInlineEdit] Forgot to add snapshot updates

* [REVERT] Add documentation example to InlineEdit Text to display isReadOnly prop

* Revert "[REVERT] Add documentation example to InlineEdit Text to display isReadOnly prop"

This reverts commit 8a7b117.

Revert change made to demo for testing

* [PR Review]
- Add an official example for the isReadOnly prop
- Name correction in edit mode props example

* [PR Feedback]
- Updated the name of Emotion style object to indicate we're styling the read mode button, not the entire form
- Conditionally hide the text content of aria-describedby when isReadOnly is true
- Conditionally add a role (and aria-level where required) to the read mode button when isReadOnly is true

* Update and add test cases

* [PR Feedback] - Update conditional aria-describedby span to surround the text instead of the entire span.
- Clean up logic related to adding aria-live and role attributes when isReadOnly is true

* Update src-docs/src/views/inline_edit/inline_edit_read_only.tsx

Co-authored-by: Cee Chen <[email protected]>

* Update src/components/inline_edit/inline_edit_form.styles.ts

Co-authored-by: Cee Chen <[email protected]>

* [PR Feedback] -Add note in inline_edit_form.style.ts that we should reevaluate styles once EuiEmptyButton is converted to Eotion.
- Updated render condition for edit mode to ensure if isReadOnly is true, we stay / kick back to read mode
- Test case & snapshot updates

* [PR Feedback] Move the isReadOnly prop example up in the documentation

* Snapshots

* Update src-docs/src/views/inline_edit/inline_edit_example.js

Co-authored-by: Cee Chen <[email protected]>

* [PR Feedback] - Removed conditiona lo logic to force a toggle from read mode to edit mode when isReadOnly is true. Opted for a useEffect that will handle the change instead

* Oops!

---------

Co-authored-by: Cee Chen <[email protected]>

* Snapshots

* [EuiInlineEdit] Final screen reader / a11y polish pass (#6805)

* Fix conditional readonly aria-describedby

- it was wrapped around the wrong SR text

* Prevent double heading roles when readonly

- leave role as undefined/unset otherwise, as `span` elements should not have a `heading` role

* (hopefully) Fix enter screen reader behavior

* Fix VO announcing EuiInlineEditText as `clickable` in read only mode

* Revert "Fix VO announcing EuiInlineEditText as `clickable` in read only mode"

This reverts commit 0f483bd.

* NVDA browser mode fix

* Revert Escape preventDefault

- not actually doing anything :wompwomp:

* [misc UX enhancement]

Set `readOnly` on input / prevent further user editing when saving & is in `isLoading` state

- docs only improvement as I don't want to tie the two props together (slightly opinionated), but hopefully consumers copy it

* [mobile] Fix edit mode buttons wrapping to next line in small screens

* [misc docs tweaks]

- add `size` props to text/title snippets

- move longer mode props snippet further down to match location on page

- reorder props in demos by relative importance/requirement (or remove if not important)

* Add demo section for `startWithEditOpen` prop

* [misc cleanup/nits]

- rename exported form styles more accurately to their usage

- test name casing, + don't use camelCasing for non props

---------

Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Elizabet Oliveira <[email protected]>
Co-authored-by: Constance Chen <[email protected]>
Co-authored-by: Trevor Pierce <[email protected]>
@breehall breehall deleted the inline-edit/readOnly branch October 6, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants