Skip to content

[EuiInlineEdit] Allow EuiInlineEdit to be used as a controlled component #7157

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.

Sign up for GitHub

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

Jump to bottom
Merged
cee-chen merged 13 commits into from
Sep 8, 2023
Merged

[EuiInlineEdit] Allow EuiInlineEdit to be used as a controlled component #7157

cee-chen merged 13 commits into from
Sep 8, 2023

Conversation

@breehall
Copy link
Contributor

@breehall breehall commented Sep 5, 2023 •
edited
Loading

Closes #7084 || Previous PR: #7117

Summary

  • AllowsEuiInlineEditText and EuiInlineEditTitle title to both be used as controlled components by creating three new props (value, onChange, onCancel).
  • Updates the props of inline edit & friends to exclusively accept defaultValue OR controlled props (value, onChange, onCancel)
  • Clean up EuiInlineEditText and EuiInlineEditTitle by removing unneeded and duplicated props.
  • Adds test cases to ensure new logic & controlled props are covered by test cases
image

QA

View the new controlled component section in the Inline Edit docs

  • Confirm you can go from read mode to edit mode by clicking on the read mode button
  • Make changes in edit mode and save -> You should find your new text, not the default Hello World!
  • Go back to edit mode to make changes and cancel -> You should find the previous text, not your most recent changes
  • There should be no visual differences in this example vs. the other inline edit sections

General checklist

  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Added documentation
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

@breehall
[Setup] Create three new props to allow inline edit to be used as a c…
39b7818 
…ontrolled component
@breehall
[EuiInlineEdit] Implement control logic
d3862c5 
- Created a DRY useMemo hook to handle switching/updating the value to be used in both read mode/edit mode and between controlled vs uncontrolled configurations
- Replace readMode/editMode state variables with useMemo value
- Pass new onChange & onCancel props to their respective functions within Inline Edit Form
@breehall
[EuiInlineEdit] Bug Fix & Cleanup
53d5b38 
- Remove the unnecessary passing of inline edit form props from EuiInlineEditText and EuiInlineEditTitle. These components we both importing props like defaultValue, placeholder, etc from InlineEditForm just send them back unchanged. The props transformed / changed within Text and Title remain while the others have been removed.
- Spread {...rest} to the InlineEditForm container div. It was being passed by Text and Title, but was never used.
- Updated snapshots to relflect the above change
@breehall
[EuiInlineEdit] Update the test cases for Inline Edit Form by creatin…
a643cc5 
…g a section dedicated to the props specific to creating a controlled component
@breehall
[InlineEdit] Create documentation for controlled component usage
08c5c30
@breehall breehall changed the title Eui inline edit/controlled component [EuiInlineEdit] Allow EuiInlineEdit to be used as a controlled component Sep 5, 2023
@breehall breehall mentioned this pull request Sep 5, 2023
Closed
11 tasks
@kibanamachine
Copy link

kibanamachine commented Sep 5, 2023

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

@breehall breehall requested a review from cee-chen September 5, 2023 22:51
@breehall breehall marked this pull request as ready for review September 5, 2023 22:51
@kibanamachine
Copy link

kibanamachine commented Sep 5, 2023

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

cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
src/components/inline_edit/inline_edit_form.tsx
import classNames from 'classnames';

import { CommonProps } from '../common';
import { CommonProps, ExclusiveUnion } from '../common';
Copy link
Contributor

@cee-chen cee-chen Sep 6, 2023

Choose a reason for hiding this comment

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

Awesome that we got ExclusiveUnion to work after all! I was a little worried it would turn out to be a headache

cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 6, 2023
View reviewed changes
@breehall
[PR Feedback]
433ff26 
- Inline Edit Text & Title - add the isReadOnly prop back (it was removed when refactoring these two child components by accident)
- Architecture updates from PR feedback
@breehall
[PR Feedback] Small update to documentation copy and example for usin…
a0dedea 
…g inline edit as a controlled component
@breehall
[Test Updates]
627b33d 
- Update unit tests to have more specific names and test assertions
- Added an additional test for onSave validation
@breehall breehall requested a review from cee-chen September 7, 2023 18:24
@kibanamachine
Copy link

kibanamachine commented Sep 7, 2023

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

cee-chen
cee-chen reviewed Sep 7, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 7, 2023
View reviewed changes
cee-chen
cee-chen reviewed Sep 7, 2023
View reviewed changes
@breehall
[PR Feedback]
a180fb1 
- Add undefined fallback for placeholder for snapshot readability
- Unit test copy and functionality updates
@kibanamachine
Copy link

kibanamachine commented Sep 7, 2023

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

breehall
breehall commented Sep 8, 2023
View reviewed changes
breehall
breehall commented Sep 8, 2023
View reviewed changes
@breehall breehall requested a review from cee-chen September 8, 2023 20:23
@kibanamachine
Copy link

kibanamachine commented Sep 8, 2023

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

cee-chen
cee-chen approved these changes Sep 8, 2023
View reviewed changes
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.

🎉 Wahoo! Thanks for taking the time to do multiple feedback rounds Bree. This approach feels clean, well-tested, and well-typed. Awesome implementation all around!

@cee-chen cee-chen enabled auto-merge (squash) September 8, 2023 21:15
@kibanamachine
Copy link

kibanamachine commented Sep 8, 2023

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

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 8, 2023

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 5972190 into elastic:main Sep 8, 2023
@breehall breehall mentioned this pull request Sep 12, 2023
Merged
jbudz added a commit to elastic/kibana that referenced this pull request Sep 18, 2023
@breehall @kibanamachine
@cee-chen @jbudz
Upgrade EUI to v88.3.0 (#166292)
b7ba000 
EUI `88.2.0` ➡️ `88.3.0`

## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0)

- `EuiGlobalToastList` now shows a "Clear all" button by default once
above a certain number of toasts (defaults to 3). This threshold is
configurable with the `showClearAllButtonAt` prop
([#7111](elastic/eui#7111))
- Added an optional `onClearAllToasts` callback to `EuiGlobalToastList`
([#7111](elastic/eui#7111))
- Added the `value`, `onChange`, and `onCancel` props that allow
`EuiInlineEdit` to be used as a controlled component
([#7157](elastic/eui#7157))
- Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`,
`transitionTopIn`, and `transitionTopOut` icon glyphs.
([#7168](elastic/eui#7168))

**Bug fixes**

- Fixed `EuiInlineEdit` components to correctly spread `...rest`
attributes to the parent wrapper
([#7157](elastic/eui#7157))
- Fixed `EuiListGroupItem` to correctly render the `extraAction` button
when `showToolTip` is also passed
([#7159](elastic/eui#7159))

**Dependency updates**

- Updated `@hello-pangea/dnd` to v16.3.0
([#7125](elastic/eui#7125))
- Updated `@types/lodash` to v4.14.198
([#7126](elastic/eui#7126))

**Accessibility**

- `EuiAccordion` now correctly respects reduced motion settings
([#7161](elastic/eui#7161))
- `EuiAccordion` now shows a focus outline to keyboard users around its
revealed children on open
([#7161](elastic/eui#7161))

**CSS-in-JS conversions**

- Converted `EuiSplitPanel` to Emotion
([#7172](elastic/eui#7172))


⚠️ As a quick heads up, serverless tests appear to have been extremely
flake/failure-prone the last couple weeks, particularly Cypress tests.
We've evaluated the listed failures and fixed ones that were related to
changes in this PR, and we're relatively confident the remaining
failures are not related to changes from EUI. Please let us know if you
think this is not the case.

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Jon <[email protected]>
@cee-chen cee-chen mentioned this pull request Sep 18, 2023
Merged
jbudz added a commit to elastic/kibana that referenced this pull request Sep 20, 2023
@cee-chen @breehall
@kibanamachine @jbudz
Upgrade EUI to v88.3.0 (#166676)
102dbe2 
⚠️ NOTE: This PR is a copy of #166292 (which was reverted due to failing
Storybook builds). This is the same exact PR but with Storybook building
fixed.

---

EUI `88.2.0` ➡️ `88.3.0`

## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0)

- `EuiGlobalToastList` now shows a "Clear all" button by default once
above a certain number of toasts (defaults to 3). This threshold is
configurable with the `showClearAllButtonAt` prop
([#7111](elastic/eui#7111))
- Added an optional `onClearAllToasts` callback to `EuiGlobalToastList`
([#7111](elastic/eui#7111))
- Added the `value`, `onChange`, and `onCancel` props that allow
`EuiInlineEdit` to be used as a controlled component
([#7157](elastic/eui#7157))
- Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`,
`transitionTopIn`, and `transitionTopOut` icon glyphs.
([#7168](elastic/eui#7168))

**Bug fixes**

- Fixed `EuiInlineEdit` components to correctly spread `...rest`
attributes to the parent wrapper
([#7157](elastic/eui#7157))
- Fixed `EuiListGroupItem` to correctly render the `extraAction` button
when `showToolTip` is also passed
([#7159](elastic/eui#7159))

**Dependency updates**

- Updated `@hello-pangea/dnd` to v16.3.0
([#7125](elastic/eui#7125))
- Updated `@types/lodash` to v4.14.198
([#7126](elastic/eui#7126))

**Accessibility**

- `EuiAccordion` now correctly respects reduced motion settings
([#7161](elastic/eui#7161))
- `EuiAccordion` now shows a focus outline to keyboard users around its
revealed children on open
([#7161](elastic/eui#7161))

**CSS-in-JS conversions**

- Converted `EuiSplitPanel` to Emotion
([#7172](elastic/eui#7172))

---------

Co-authored-by: Bree Hall <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Jon <[email protected]>
@breehall breehall deleted the EuiInlineEdit/controlled-component branch October 6, 2023 16:02
@CoenWarmer
Copy link

CoenWarmer commented Oct 26, 2023

Thanks for this @breehall!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cee-chen cee-chen cee-chen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiInlineEdit] Create value prop for full control over inline edit text and title values

5 participants

@breehall @kibanamachine @elasticmachine @CoenWarmer @cee-chen