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

[Cases] Replace EditableTitle component with EuiInlineEdit component #162095

Merged
merged 17 commits into from
Aug 1, 2023

Conversation

breehall
Copy link
Contributor

@breehall breehall commented Jul 17, 2023

Included in elastic/eui#6778

Hi team! EUI recently released the EuiInlineEdit component and the Cases page title was identified as a good candidate for the new component. This PR is replaces the inner workings of the EditableTitle component and replaces it with the new EuiInlineEdit component.

Summary

Replace inner component within EditableTitle to use to the new EuiInlineEdit component.

Read Mode
image


Edit Mode
image


Insufficient Permissions
image


Error States
image

image

Release Phases
image

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Comment on lines +31 to 32
export const TitleExperimentalBadge = React.memo(ExperimentalBadge);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to export these as individual components to also be used in EditableTitle (Cases) instead of removing it so I don't disturb other usages.

@@ -87,7 +87,7 @@ const HeaderPageComponent: React.FC<HeaderPageProps> = ({
return (
<Header border={border} data-test-subj={dataTestSubj}>
<EuiFlexGroup alignItems="center">
<FlexItem>
<FlexItem css={{ overflow: 'hidden' }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required for the overflow of very long case titles.

return (
<>
<EuiFlexGroup>
<EuiFlexItem grow={true} css={releasePhase && { overflow: 'hidden' }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Truncate the case title when a releasePhase is present so both the title and beta badge can be displayed next to each other on the same line.

@adcoelho
Copy link
Contributor

I played around a bit with it and it seems to work fine and cover the functionality we need 👍 👍
(Pinging @mdefazio for the design differences to the previous version)

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Hi Bree,
Thanks a lot for the new component!! I love it. ❤️

I tested different scenarios and they work as expected. 🎉

Just a minor point:
current case title is 34 px, while new component's title is around 22 px.

@mdefazio
Copy link
Contributor

I'm good with the smaller font size. Thanks for updating this--works great!

@breehall
Copy link
Contributor Author

@elasticmachine merge upstream

@breehall breehall marked this pull request as ready for review July 25, 2023 20:15
@breehall breehall requested review from a team as code owners July 25, 2023 20:15
@breehall breehall changed the title [Cases] (WIP) Replace EditableTitle component with EuiInlineEdit component [Cases] Replace EditableTitle component with EuiInlineEdit component Jul 25, 2023
@breehall breehall added release_note:skip Skip the PR/issue when compiling release notes EUI labels Jul 25, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@breehall breehall requested a review from js-jankisalvi July 27, 2023 13:57
@cnasikas cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature labels Jul 28, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Verified locally, changes look good 👍

Few questions:

  1. In cases page form fields disable the save button when element has errors.
    We have an open issue to align all case view page fields with same behaviour: Disable submit if the form has errors
    Do you think we can disable save button when title has errors?
    CC: @cnasikas @mdefazio

  2. When there is an error in the title and user changes the value, shouldn't the error message be removed?

title.mov

return;
}
if (newTitleValue !== title) {
onSubmit(newTitleValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

Suggested change
onSubmit(newTitleValue);
onSubmit(newTitleValue.trim());

It looks a bit weird that when editing the spaces are displayed but when showing the title they aren't.

Screen.Recording.2023-07-28.at.15.52.41.mov

What was it we decided on this @js-jankisalvi @cnasikas ?

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 we discussed to handle it separately because current production behaviour is same as here.
However it is a small change so I am okay if Bree wants to make a change in this PR.

iconType="save"
onClick={onClickSubmit}
return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this fragment is unnecessary because we are only returning EuiFlexGroup 😅

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Tested and looks good 👍

Just wondering about trimming the text.

breehall added 3 commits July 28, 2023 10:29
- Remove fragment surrounding inline edit component
- Trim title before saving
- Add onChange listener to input to reset errors upon input change
…o InlineEdit/obs_cases

Pulling in changes made from Gihub
@breehall
Copy link
Contributor Author

@adcoelho @js-jankisalvi

In my latest commit (56dabbd), I added a few tweaks and also updated the error message handling so the message is removed when the user starts typing again. The message will return if the input is invalid at save.

Screen.Recording.2023-07-28.at.10.46.37.AM.mov

@cnasikas
Copy link
Member

In cases page form fields disable the save button when element has errors.
We have #161461 to align all case view page fields with same behaviour: Disable submit if the form has errors
Do you think we can disable save button when title has errors?
CC: @cnasikas @mdefazio

I think we can move forward and merge the PR as it is and then fix it ourselves when we start working on the issue.

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Awesome work 🎉

@cee-chen
Copy link
Contributor

@elasticmachine merge upstream

@cee-chen
Copy link
Contributor

@cnasikas, @js-jankisalvi, thank you so much for your time and reviews! Bree is out today/tomorrow so I'll go ahead and merge on her behalf once CI passes. Please feel free to tweak UX & implementation in the way your team prefers, and thanks for being our first adopters of EuiInlineEdit!

@cee-chen cee-chen enabled auto-merge (squash) July 31, 2023 15:31
@cee-chen
Copy link
Contributor

Oh, wait, I just noticed we're still missing one pending approval/review from @elastic/security-solution - any idea who to ping for that? :)

@cee-chen
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine kibanamachine requested a review from a team as a code owner July 31, 2023 17:06
@cee-chen
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Investigations - Security Solution Cypress Tests #1 / Alert details expandable flyout right panel overview tab insights section should display threat intelligence section should display threat intelligence section

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cases 421.7KB 421.1KB -533.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Threat Hunting - Explore changes LGTM!

@cee-chen cee-chen merged commit 8c7c621 into elastic:main Aug 1, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 1, 2023
@cee-chen
Copy link
Contributor

cee-chen commented Aug 1, 2023

Woo hoo, thanks y'all! :)

ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
…ent (elastic#162095)

Included in elastic/eui#6778

Hi team! EUI recently released the EuiInlineEdit component and the Cases
page title was identified as a good candidate for the new component.
This PR is replaces the inner workings of the EditableTitle component
and replaces it with the new EuiInlineEdit component.

## Summary
Replace inner component within `EditableTitle` to use to the new
`EuiInlineEdit` component.


**Read Mode**
<img width="1116" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/c3aef300-7d7f-44cd-b0a2-da72ef8bedf9">

---

**Edit Mode**
<img width="1093" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/0567ea9b-29e4-4443-bcbe-7a45f09738ff">

---

**Insufficient Permissions**
<img width="1090" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/7a83ebc6-7319-415d-a4a8-015fd6f6893b">

---

**Error States**
<img width="1093" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/1e5360b0-4fe8-4a25-a5e2-d3b235fc6242">

<img width="1108" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/ecfdcdc5-9360-4d25-8a4b-7132ec2caa67">

---

**Release Phases**
<img width="1096" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/cb0ac70b-1ba2-4f3a-8966-25b38043c4a1">

<img width="1096" alt="image"
src="https://github.com/elastic/kibana/assets/40739624/23597578-0a36-4190-8e84-804cecbbdf78">

---


### Checklist

Delete any items that are not applicable to this PR.

- [ ] ~Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)~
- [ ]
~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials~
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] ~If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants