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

[CCR & Snapshot+Restore] Center align states under tabs #103237

Merged
merged 16 commits into from
Jun 29, 2021
Merged

[CCR & Snapshot+Restore] Center align states under tabs #103237

merged 16 commits into from
Jun 29, 2021

Conversation

sabarasaba
Copy link
Member

@sabarasaba sabarasaba commented Jun 24, 2021 •
edited
Loading

Addresses remaining work that was discussed #102507 (comment)

I also found a small bug in the ILM app, and since it was such a small fix I ended up doing it here also 🙈

Related: #100748

Screenshots CCR

Follower Indices Tab

Screenshot 2021-06-24 at 13 09 43Screenshot 2021-06-24 at 13 10 02Screenshot 2021-06-24 at 13 09 06
Screenshot 2021-06-24 at 13 07 06
Screenshot 2021-06-24 at 13 15 26

Auto-follow Patterns Tab

Screenshot 2021-06-24 at 13 12 05
Screenshot 2021-06-24 at 13 11 40
Screenshot 2021-06-24 at 13 12 30
Screenshot 2021-06-24 at 13 12 42
Screenshot 2021-06-24 at 13 14 08

Screenshots Snapshot+Restore

Repositories Tab

Screenshot 2021-06-24 at 12 52 10
Screenshot 2021-06-24 at 12 52 53
Screenshot 2021-06-24 at 12 53 40
Screenshot 2021-06-24 at 12 53 57

Policies Tab

Screenshot 2021-06-24 at 12 56 58
Screenshot 2021-06-24 at 12 56 00
Screenshot 2021-06-24 at 12 56 26
Screenshot 2021-06-24 at 12 56 44

Snapshots Tab

Screenshot 2021-06-24 at 12 59 17
Screenshot 2021-06-24 at 12 57 55
Screenshot 2021-06-24 at 12 58 43
Screenshot 2021-06-24 at 12 59 02
Screenshot 2021-06-24 at 12 59 35
Screenshot 2021-06-24 at 12 59 58

Restore Tab

Screenshot 2021-06-24 at 13 01 18
Screenshot 2021-06-24 at 13 00 56
Screenshot 2021-06-24 at 13 01 47
Screenshot 2021-06-24 at 13 02 04

@sabarasaba sabarasaba self-assigned this Jun 24, 2021
@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@sabarasaba sabarasaba added chore release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels Jun 24, 2021
@sabarasaba sabarasaba added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Jun 24, 2021
@sabarasaba sabarasaba marked this pull request as ready for review June 24, 2021 12:46
@sabarasaba sabarasaba requested a review from a team as a code owner June 24, 2021 12:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

alisonelizabeth
alisonelizabeth reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks so much for working on this @sabarasaba! The screens look great - nice to see all of this coming together and aligning on patterns across our apps.

I noticed one issue with the text rendering outside the error prompt in Snapshot + Restore (although this may be an EUI issue).

Screen Shot 2021-06-24 at 10 29 09 AM

[EDITED] I also have a couple concerns around the refactoring made to Snapshot + Restore. First of all, I agree returning early is the better approach and aligns with some of the comments Seb made in #101548. That said, in a couple places with your current changes (tried to make comments where applicable) I think we lose the privileges check. I think we'd always want this check to happen.

I would also add that Snapshot + Restore is known for having some gaps in test coverage (old issue, but mostly still relevant: #47595). I would just lean more on the cautious side when refactoring the code and focus on the minimal changes needed to align the UX until we have more robust test coverage.

...plugins/index_lifecycle_management/public/application/sections/policy_table/policy_table.tsx Show resolved Hide resolved
x-pack/plugins/snapshot_restore/public/application/sections/home/restore_list/restore_list.tsx Show resolved Hide resolved
...er_replication/public/app/sections/home/auto_follow_pattern_list/auto_follow_pattern_list.js Show resolved Hide resolved
x-pack/plugins/snapshot_restore/public/application/sections/home/policy_list/policy_list.tsx Show resolved Hide resolved
@alisonelizabeth
Copy link
Contributor

One more thing 😄

Do you think this screen should be shown as an error, rather than just the subdued empty prompt?

Screenshot

@sabarasaba
Copy link
Member Author

Thanks for having a look @alisonelizabeth! I think I agree with you, lets better defer the refactoring for later and stick to the little ux changes for now. I've reverted the changes back to using the content var as it was before. I've also made a few more changes as suggested:

  • Converted the snapshots list -> some repositories contains errors to an actual error overlay
  • Wrapped the PageError paragraph that shows the error message with an eui-textBreakWord class so that if a message is long enough, it wont overflow out of the container

Screenshot 2021-06-28 at 09 27 55

@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

alisonelizabeth
alisonelizabeth approved these changes Jun 28, 2021
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.

Thanks for making those changes @sabarasaba! Latest LGTM. Looks like there's a small prettier error causing CI to fail, but happy to see this merged once that's addressed.

@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
crossClusterReplication 290.5KB 290.3KB -228.0B
indexLifecycleManagement 248.6KB 248.6KB -33.0B
snapshotRestore 458.9KB 460.2KB +1.3KB
total +1.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
crossClusterReplication 24.9KB 25.1KB +230.0B
esUiShared 192.5KB 192.6KB +27.0B
snapshotRestore 40.9KB 41.1KB +166.0B
total +423.0B

History

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

cc @sabarasaba

@sabarasaba sabarasaba merged commit 09bd630 into elastic:master Jun 29, 2021
@sabarasaba sabarasaba mentioned this pull request Jun 29, 2021
Merged
sabarasaba added a commit to sabarasaba/kibana that referenced this pull request Jun 29, 2021
@sabarasaba @kibanamachine
[CCR & Snapshot+Restore] Center align states under tabs (elastic#103237)
0f5cea1 
* fix up CCR centered sates in tabs content

* update snapshots list

* fix lint errors

* Change tab states for all pages in snapshot+restore

* Remove unnecessary variables

* Seems we dont need the class wrapper

* fix broken type

* Fix bug in ILM table when filtering it down

* center align search box

* fix linter errors

* fix prettier warnings

* revert content var refactor and just focus on ux

* add breakword class to paragraph so we avoid text overflowing

* fix prettier errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
sabarasaba added a commit that referenced this pull request Jun 29, 2021
@sabarasaba @kibanamachine
[CCR & Snapshot+Restore] Center align states under tabs (#103237) (#1…
a6ebd0b 
…03610)

* fix up CCR centered sates in tabs content

* update snapshots list

* fix lint errors

* Change tab states for all pages in snapshot+restore

* Remove unnecessary variables

* Seems we dont need the class wrapper

* fix broken type

* Fix bug in ILM table when filtering it down

* center align search box

* fix linter errors

* fix prettier warnings

* revert content var refactor and just focus on ux

* add breakword class to paragraph so we avoid text overflowing

* fix prettier errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@alisonelizabeth alisonelizabeth mentioned this pull request Jun 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees

@sabarasaba sabarasaba

Labels
chore 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.
4 participants
@sabarasaba @elasticmachine @alisonelizabeth @kibanamachine