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] Copy update #91100

Merged
merged 4 commits into from
Feb 15, 2021
Merged

[ILM] Copy update #91100

merged 4 commits into from
Feb 15, 2021

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Feb 11, 2021 •
edited
Loading

Summary

This PR continues work done in 88024, 88671, 90291.

Previous work concentrated on aligning ILM design with data tiers concept and this PR contains a copy update concerning:

  • changes of phase descriptions to mention nodes and cost savings
  • changes 'delete phase' button wording to active voice
  • fixes for wrong capitalization
  • swaps save/cancel buttons into correct order

Screenshot

Screenshot 2021-02-11 at 15 45 00

Checklist

@yuliacech yuliacech requested a review from a team as a code owner February 11, 2021 10:37
@yuliacech yuliacech added Feature:ILM Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.12.0 v8.0.0 labels Feb 11, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@yuliacech yuliacech added the release_note:skip Skip the PR/issue when compiling release notes label Feb 11, 2021
@yuliacech yuliacech requested review from jloleysens and a team February 11, 2021 10:38
jloleysens
jloleysens approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

These changes look good @yuliacech ! I did not test locally, happy when CI is passing. Would you mind adding a screenshot of the buttons that have swapped position?

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @jloleysens! I updated the screenshot with a full form version :)

jrodewig
jrodewig previously requested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks for raising this @yuliacech.

While most of the changes look good, I have some major concerns regarding how the current UI uses "data" in place of "index." In ES, "data" could mean document or index.

The UI should be clear that the policy affects indices, not documents. For example:

Screen Shot 2021-02-11 at 11 26 08 AM

Users could interpret this copy as:

"The policy moves documents that are N days old."
OR
"The policy moves documents with a timestamp that is N days old."

But neither is the case. Phase migration is triggered by index age or rollover date.

I'd also like to get feedback from @debadair, who has worked closely on ILM in the past.

..._management/public/application/sections/edit_policy/components/phase_footer/phase_footer.tsx Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
..._management/public/application/sections/edit_policy/components/phase_footer/phase_footer.tsx Outdated Show resolved Hide resolved
debadair
debadair suggested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

I left some alternate suggestions that are more from the viewpoint of moving data between the data tiers than the underlying mechanics. I think it would be good to get @gchaps to weigh in on the terminology.

..._management/public/application/sections/edit_policy/components/phase_footer/phase_footer.tsx Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts Outdated Show resolved Hide resolved
..._management/public/application/sections/edit_policy/components/phase_footer/phase_footer.tsx Outdated Show resolved Hide resolved
@jrodewig jrodewig dismissed their stale review February 12, 2021 18:32

Stale review

@jrodewig
Copy link
Contributor

Thanks for the additional thoughts @debadair @gchaps

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

kibanamachine and others added 3 commits February 15, 2021 06:16
@kibanamachine
Merge branch 'master' into ilm_copy
5813d19
@yuliacech @jrodewig @debadair
Apply suggestions from code review
c806392 
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
@yuliacech
Fixed eslint issues and added labels for buttons
d2f49db
jrodewig
jrodewig approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @yuliacech.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
indexLifecycleManagement 240.6KB 240.2KB -371.0B

History

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

@yuliacech
Copy link
Contributor Author

Thanks a lot for such great input, team! @jrodewig @debadair @gchaps

@yuliacech yuliacech merged commit 91d3a6a into elastic:master Feb 15, 2021
@yuliacech yuliacech mentioned this pull request Feb 15, 2021
Merged
yuliacech added a commit to yuliacech/kibana that referenced this pull request Feb 15, 2021
@yuliacech @kibanamachine
@jrodewig @debadair
[ILM] Copy update (elastic#91100)
1fe456f 
* Added copy adjustments, fixed wrong capitalization of titles and swap primary buttons back to correct order

* Apply suggestions from code review

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>

* Fixed eslint issues and added labels for buttons

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
yuliacech added a commit that referenced this pull request Feb 16, 2021
@yuliacech @kibanamachine
@jrodewig @debadair
[ILM] Copy update (#91100) (#91425)
fe84a64 
* Added copy adjustments, fixed wrong capitalization of titles and swap primary buttons back to correct order

* Apply suggestions from code review

Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>

* Fixed eslint issues and added labels for buttons

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>
Co-authored-by: debadair <debadair@elastic.co>
@yuliacech yuliacech deleted the ilm_copy branch February 24, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gchaps gchaps gchaps left review comments

@debadair debadair debadair requested changes

@jloleysens jloleysens jloleysens approved these changes

@jrodewig jrodewig jrodewig approved these changes

Assignees
No one assigned
Labels
Feature:ILM 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 v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@yuliacech @elasticmachine @jrodewig @kibanamachine @debadair @jloleysens @gchaps