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.

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

[ILM] Refactor edit policy form to use EuiTimeline #131732

Merged
merged 13 commits into from
May 18, 2022
Merged

[ILM] Refactor edit policy form to use EuiTimeline #131732

merged 13 commits into from
May 18, 2022

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented May 6, 2022 •
edited
Loading

Summary

Fixes #130302

This PR changes the edit policy form to use EuiTimeline and EuiTimelineItem instead of EuiComment. There should be no visual changes to the UI or functionality of the edit form.

Screenshot

Screenshot 2022-05-12 at 13 20 44

@yuliacech yuliacech added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes labels May 6, 2022
@yuliacech
Copy link
Contributor Author

Hi @cristina-eleonora, could you please have a look at this draft PR?
I updated the code and now the vertical alignment of the phase icon and the phase panel changed. Previously, we tried to align the icon to the phase toggle (even though the alignment didn't work on smaller screens). Now we can only align the icon either to the top of the phase or in the middle of the panel (see screenshots below for before/after).

Before
Screenshot 2022-05-06 at 16 41 51

After
Screenshot 2022-05-06 at 16 34 12

cristina-eleonora
cristina-eleonora reviewed May 10, 2022
View reviewed changes
Copy link

@cristina-eleonora cristina-eleonora left a comment

Choose a reason for hiding this comment

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

Pulled and tested. In comparison with the other components, it appears that the timeline icons are located slightly higher than before.

Screenshot 2022-05-10 at 13 33 26

The icon, the switch, the title and the badge, as well as the Move data into phase when fields should all be center aligned.

@yuliacech yuliacech marked this pull request as ready for review May 12, 2022 11:40
@yuliacech yuliacech requested review from a team as code owners May 12, 2022 11:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

alisonelizabeth
alisonelizabeth reviewed May 12, 2022
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Great to see this converted to EuiTimeline 🎉 . Nice work!

I noticed a small difference with the panel header. It looks like we lost the space between the phase title and the inputs. Also, the age field looks a lot wider.

Before
Screen Shot 2022-05-12 at 9 21 33 AM

After
Screen Shot 2022-05-12 at 9 21 52 AM

@yuliacech
Copy link
Contributor Author

Thanks a lot for your reviews, @cristina-eleonora and @alisonelizabeth!
I fixed the issues you mentioned, could you please have another look?

alisonelizabeth
alisonelizabeth approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Latest LGTM. Thanks @yuliacech!

cristina-eleonora
cristina-eleonora approved these changes May 18, 2022
View reviewed changes
Copy link

@cristina-eleonora cristina-eleonora left a comment

Choose a reason for hiding this comment

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

Pulled and tested, and it looks great. Thanks Yulia!

@yuliacech yuliacech enabled auto-merge (squash) May 18, 2022 12:15
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
indexLifecycleManagement 182 187 +5

Async chunks

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

id before after diff
indexLifecycleManagement 148.4KB 148.2KB -198.0B

History

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

@yuliacech yuliacech merged commit 57aebc4 into elastic:main May 18, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 18, 2022
@yuliacech yuliacech deleted the ilm_timeline branch November 22, 2022 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristina-eleonora cristina-eleonora cristina-eleonora approved these changes

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ILM] Update edit policy form to use EuiTimeline instead of EuiComment
6 participants
@yuliacech @elasticmachine @kibana-ci @alisonelizabeth @cristina-eleonora @kibanamachine