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

[Upgrade Assistant] Address copy feedback #99632

Merged
merged 8 commits into from
May 25, 2021
Merged

[Upgrade Assistant] Address copy feedback #99632

merged 8 commits into from
May 25, 2021

Conversation

alisonelizabeth
Copy link
Contributor

This PR addresses copy feedback in #97159.

Screen Shot 2021-05-10 at 9 46 30 AM

Screen Shot 2021-05-10 at 9 46 55 AM

Screen Shot 2021-05-10 at 9 47 07 AM

@alisonelizabeth alisonelizabeth added v8.0.0 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 Feature:Upgrade Assistant v7.14.0 labels May 10, 2021
alisonelizabeth
alisonelizabeth commented May 10, 2021
View reviewed changes
...k/plugins/upgrade_assistant/public/application/components/shared/search_bar/level_filter.tsx
@@ -13,15 +13,9 @@ import { i18n } from '@kbn/i18n';
import { LevelFilterOption } from '../../types';

const LocalizedOptions: { [option: string]: string } = {
warning: i18n.translate(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not being used anymore

@alisonelizabeth alisonelizabeth mentioned this pull request May 10, 2021
Merged
alisonelizabeth
alisonelizabeth commented May 10, 2021
View reviewed changes
...lugins/upgrade_assistant/public/application/components/shared/search_bar/group_by_filter.tsx
@@ -14,10 +14,10 @@ import { GroupByOption } from '../../types';

const LocalizedOptions: { [option: string]: string } = {
message: i18n.translate('xpack.upgradeAssistant.checkupTab.controls.groupByBar.byIssueLabel', {
defaultMessage: 'by issue',
defaultMessage: 'By issue',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@debadair since you suggested capitalizing Critical and Warning, it seemed like we should also capitalize the By issue and By index filters on the ES deprecations page. Let me know if you disagree.

@alisonelizabeth alisonelizabeth marked this pull request as ready for review May 10, 2021 14:18
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner May 10, 2021 14:18
@elasticmachine
Copy link
Contributor

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

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

debadair
debadair approved these changes May 25, 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.

These changes LGTM.

I'm assuming the steps to resolve are placeholder text, but a couple things to note for future reference:

  • Use bold instead of quotes for emphasis when directing the user to interact with a UI element.
  • We generally use "Go to xyz" instead of "Navigate to".

In the Elasticsearch section, I think I'd explicitly recommend backing up your data before making changes. IOW, add "Back up your data before making changes to your Elasticsearch configuration." As it is, the back up button just feels sort of stuck there.

Similar to the Kibana issues list, it would be good if the Elasticsearch issues could specify the affected index.

@alisonelizabeth
Copy link
Contributor Author

Thanks @debadair for the review!

I'm assuming the steps to resolve are placeholder text, but a couple things to note for future reference:

This text is provided by the plugin owner responsible for registering the deprecation. I'll make a note of this and reach out the timelion team about updating it.

In the Elasticsearch section, I think I'd explicitly recommend backing up your data before making changes. IOW, add "Back up your data before making changes to your Elasticsearch configuration." As it is, the back up button just feels sort of stuck there.

👍 thanks for the feedback. I think it was originally text w/ a link (for the 6-7 upgrade), and we moved toward the button only as part of the redesign. /cc @dborodyansky

Similar to the Kibana issues list, it would be good if the Elasticsearch issues could specify the affected index.

I'm not sure I follow this comment. Are you referring to the including the index name as part of the description? If so, Dmitry is working on some changes to the layout and I think this concern will be addressed.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

cjcenizal
cjcenizal approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal 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 the screenshots -- this is looking so darn good! Code LGTM, didn't test locally.

@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
upgradeAssistant 150.4KB 149.8KB -692.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 386 342 -44
stackAlerts 101 95 -6
total -342

History

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

@alisonelizabeth alisonelizabeth merged commit a0ddca8 into elastic:master May 25, 2021
@alisonelizabeth alisonelizabeth deleted the ua/copy-fixes branch May 25, 2021 18:14
@alisonelizabeth alisonelizabeth mentioned this pull request May 25, 2021
Merged
alisonelizabeth added a commit that referenced this pull request May 25, 2021
@alisonelizabeth
[Upgrade Assistant] Address copy feedback (#99632) (#100580)
fa7dc5c
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 26, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/ua-…
812f82c 
…deprecation-ilm-policy

* 'master' of github.com:elastic/kibana: (101 commits)
  [ftr] migrate "docTable" service to FtrService class (elastic#100595)
  [ftr] migrate "listingTable" service to FtrService class (elastic#100606)
  Fixed comparing real value with formatted according to mode. (elastic#100456)
  [ftr] migrate "dataGrid" service to FtrService class (elastic#100593)
  [ftr] migrate "fieldEditor" to FtrService class (elastic#100597)
  [ftr] migrate "filterBar" service to FtrService class (elastic#100601)
  [triggersActionsUi] Reduce page load bundle to under 100kB (elastic#97770)
  [build] Clean jest configs (elastic#100594)
  refact(NA): remove extra pkg_npm target and add specific target folders for @kbn/analytics on Bazel (elastic#100569)
  Update dependency @elastic/charts to v29.2.0 (elastic#100587)
  [Maps] convert LayerPanel to typescript (elastic#100481)
  [Upgrade Assistant] Address copy feedback (elastic#99632)
  Open/Closed filter for observability alerts page (elastic#99217)
  One liner to expose the EQL query for debugging for users (elastic#100565)
  [KibanaPageLayout] Solution Nav specific styles & props (elastic#100089)
  [ftr] implement FtrService classes and migrate common services (elastic#99546)
  [XY] [Lens] Adds opacity slider (elastic#100453)
  [Reporting] ILM policy for managing reporting indices (elastic#100130)
  [Reporting] ILM policy for managing reporting indices (elastic#100130)
  [DOCS] Remove redundant maps attribute (elastic#100426)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/store/report_ilm_policy.ts
#	x-pack/plugins/reporting/server/lib/store/store.test.ts
#	x-pack/plugins/reporting/server/lib/store/store.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

@debadair debadair debadair approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@alisonelizabeth @elasticmachine @kibanamachine @debadair @cjcenizal