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

[Share] Cleanup share model UI and warning logic #196401

Merged
merged 6 commits into from
Oct 23, 2024
Merged

[Share] Cleanup share model UI and warning logic #196401

merged 6 commits into from
Oct 23, 2024

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Oct 15, 2024 •
edited by kibanamachine
Loading

Summary

This PR does a number of cleanup tasks relating to the share modal and tabs, particularly related to the export tab contents. Changes include:

  • Export tab content
    • Use default helpText and generateExportButton, now does not change based on selected option.
    • Use consistent spacing around messages
    • Use consistent styles from Links tab
    • Actually render warnings below dirty warning message
    • Only display dirty warning when a url is present. This warning does not make sense when the only action is to download a csv file.
    • Remove unused properties including isDisabled and showRadios
  • Remove unused DownloadPanelContent component.

See PR comments for more details

New changes demo

Zight.Recording.2024-10-15.at.12.27.33.PM.mp4

Checklist

@nickofthyme nickofthyme added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 15, 2024
nickofthyme
nickofthyme commented Oct 15, 2024
View reviewed changes
Copy link
Contributor Author

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Some notes about changes...

packages/kbn-reporting/public/share/share_context_menu/register_pdf_png_modal_reporting.tsx Show resolved Hide resolved
src/plugins/share/public/components/tabs/export/export_content.tsx Show resolved Hide resolved
src/plugins/share/public/components/tabs/export/export_content.tsx Show resolved Hide resolved
src/plugins/share/public/components/tabs/export/export_content.tsx Show resolved Hide resolved
src/plugins/share/public/components/tabs/export/export_content.tsx Show resolved Hide resolved
src/plugins/share/public/types.ts Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/csv_download_provider/csv_download_panel_content.tsx Outdated Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/csv_download_provider/csv_download_provider.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/csv_download_provider/csv_download_provider.tsx Show resolved Hide resolved
x-pack/plugins/lens/public/app_plugin/csv_download_provider/csv_download_provider.tsx Show resolved Hide resolved
@nickofthyme
Copy link
Contributor Author

nickofthyme commented Oct 15, 2024 •
edited
Loading

@elastic/appex-sharedux & @elastic/platform-design are you okay with these chagnes?

@MichaelMarcialis
Copy link
Contributor

@elastic/appex-sharedux & @elastic/platform-design are you okay with these chagnes?

Changes seem fine to me. CCing @ryankeairns, as I recall him being involved with the share modal in the past (though I may be mis-remembering). There's some additional wording and UX tweaks that I might recommend now that I'm looking at this closer, but I can create a separate issue for that. This looks like an improvement regardless. Thanks, @nickofthyme!

@ryankeairns
Copy link
Contributor

+1 what Michael said. Thanks for tidying this up, :shipit:

@nickofthyme
Copy link
Contributor Author

@MichaelMarcialis if your updates are pretty minor I could add them to this PR. Just lmk, I still need to update a few tests.

@nickofthyme nickofthyme marked this pull request as ready for review October 17, 2024 23:29
@nickofthyme nickofthyme requested review from a team as code owners October 17, 2024 23:29
tsullivan
tsullivan reviewed Oct 19, 2024
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Glad to see the cleanup of warning logic and unused components! I left a few comments that might help even more.

@tsullivan tsullivan mentioned this pull request Oct 19, 2024
Open
@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis if your updates are pretty minor I could add them to this PR. Just lmk, I still need to update a few tests.

Thanks, @nickofthyme! I'm stuck wrapping up a few other priorities and it looks like I might have a few additional recommendations beyond wording, so let's do a separate issue. Will CC you on it when I get it written up.

markov00
markov00 approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

All good to me, thanks the cleanup

src/plugins/share/public/components/tabs/export/export_content.tsx Outdated Show resolved Hide resolved
@nickofthyme nickofthyme enabled auto-merge (squash) October 23, 2024 16:29
@nickofthyme nickofthyme merged commit 67de924 into elastic:main Oct 23, 2024
24 checks passed
@nickofthyme nickofthyme deleted the share-modal-cleanup branch October 23, 2024 20:29
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11487644931

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
share 72 73 +1

Page load bundle

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

id before after diff
lens 51.5KB 51.4KB -146.0B
reporting 53.5KB 52.9KB -596.0B
share 57.2KB 57.6KB +416.0B
total -326.0B
Unknown metric groups

API count

id before after diff
share 135 136 +1

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 23, 2024
@nickofthyme
[Share] Cleanup share model UI and warning logic (elastic#196401)
08fbbb4 
cleanup share modal UI with better context around warnings and styles

(cherry picked from commit 67de924)
@kibanamachine kibanamachine mentioned this pull request Oct 23, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 23, 2024
@kibanamachine @nickofthyme
[8.x] [Share] Cleanup share model UI and warning logic (#196401) (#19…
9a0500d 
…7532)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Share] Cleanup share model UI and warning logic
(#196401)](#196401)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nick
Partridge","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-23T20:29:35Z","message":"[Share]
Cleanup share model UI and warning logic (#196401)\n\ncleanup share
modal UI with better context around warnings and
styles","sha":"67de9246a7d0f718e7aa97d8edbf569597964ebf","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor"],"title":"[Share]
Cleanup share model UI and warning
logic","number":196401,"url":"https://github.com/elastic/kibana/pull/196401","mergeCommit":{"message":"[Share]
Cleanup share model UI and warning logic (#196401)\n\ncleanup share
modal UI with better context around warnings and
styles","sha":"67de9246a7d0f718e7aa97d8edbf569597964ebf"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196401","number":196401,"mergeCommit":{"message":"[Share]
Cleanup share model UI and warning logic (#196401)\n\ncleanup share
modal UI with better context around warnings and
styles","sha":"67de9246a7d0f718e7aa97d8edbf569597964ebf"}}]}]
BACKPORT-->

Co-authored-by: Nick Partridge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@tsullivan tsullivan tsullivan approved these changes

Assignees
No one assigned
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.17.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nickofthyme @MichaelMarcialis @ryankeairns @kibanamachine @elasticmachine @tsullivan @markov00