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

Simplify deleting spaces #99960

Merged
merged 7 commits into from
May 18, 2021
Merged

Conversation

thomheymann
Copy link
Contributor

@thomheymann thomheymann commented May 12, 2021

Resolves #49856

Summary

This PR makes it easier to delete spaces by removing the requirement to type the name of the space in the confirmation modal. This caused issues when the space contained double spaces and was cumbersome for long space names. We already ask the user to confirm if they want to proceed which gives enough protection.

Deleting space confirmation modal

Edge case when deleting current space

Checklist

Delete any items that are not applicable to this PR.

@thomheymann thomheymann added release_note:enhancement v8.0.0 v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 12, 2021
@thomheymann thomheymann marked this pull request as ready for review May 12, 2021 19:43
@thomheymann thomheymann requested review from a team as code owners May 12, 2021 19:43
@legrego legrego self-requested a review May 13, 2021 18:27
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Tested locally -- LGTM with one nit below

'xpack.spaces.management.confirmDeleteModal.confirmButton',
{
defaultMessage:
'{isLoading, select, true{Deleting space and all contents…} other{Delete space and all contents}}',
Copy link
Member

Choose a reason for hiding this comment

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

You fancy! 🏅


import type { InjectedIntl } from '@kbn/i18n/react';
import { FormattedMessage, injectI18n } from '@kbn/i18n/react';
import { EuiCallOut, EuiConfirmModal, EuiSpacer, EuiText } from '@elastic/eui';
Copy link
Member

Choose a reason for hiding this comment

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

Using the base EuiConfirmModal here makes me so happy. Thanks for taking the time to clean this up! It had a great side-effect of reducing our async chunk size too:

image

@thomheymann
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
spaces 258 249 -9

Async chunks

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

id before after diff
spaces 288.1KB 272.8KB -15.3KB

Page load bundle

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

id before after diff
spaces 41.5KB 41.6KB +108.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
maps 286 208 -78
ml 121 115 -6
total -236

History

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

@thomheymann thomheymann merged commit 574b455 into elastic:master May 18, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 99960

@legrego legrego added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label May 18, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

thomheymann added a commit to thomheymann/kibana that referenced this pull request May 18, 2021
* Simplify deleting spaces

* Fixed i18n

* Fix functional tests

* Update x-pack/plugins/spaces/public/management/spaces_management_app.tsx

Co-authored-by: Larry Gregory <[email protected]>

* Fix snapshots

Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/spaces/public/management/spaces_management_app.tsx
thomheymann added a commit that referenced this pull request May 18, 2021
* Simplify deleting spaces

* Fixed i18n

* Fix functional tests

* Update x-pack/plugins/spaces/public/management/spaces_management_app.tsx

Co-authored-by: Larry Gregory <[email protected]>

* Fix snapshots

Co-authored-by: Larry Gregory <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/spaces/public/management/spaces_management_app.tsx
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
* Simplify deleting spaces

* Fixed i18n

* Fix functional tests

* Update x-pack/plugins/spaces/public/management/spaces_management_app.tsx

Co-authored-by: Larry Gregory <[email protected]>

* Fix snapshots

Co-authored-by: Larry Gregory <[email protected]>
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
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting a space is difficult with multiple whitespace characters
5 participants