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

[Files] Move <Image /> component to @kbn/shared-ux package #145995

Merged
merged 23 commits into from
Nov 24, 2022
Merged

[Files] Move <Image /> component to @kbn/shared-ux package #145995

merged 23 commits into from
Nov 24, 2022

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Nov 22, 2022 •
edited
Loading

Summary

  • Creates new packages @kbn/shared-ux-file-image, @kbn/shared-ux-file-image-types, @kbn/shared-ux-file-image-mocks
  • Replace all instance of usage to import from package (for now, just files example plugin)
  • Created a @kbn/shared-ux-file-util package for the helpers shared across components

How to test

See yarn storbook shared_ux's new section "Files"

Additional notes

First step just focussed on moving Image component, we still need to move FilePicker and UploadFile

@jloleysens jloleysens added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) feature:Files v8.7.0 labels Nov 22, 2022
@jloleysens jloleysens requested a review from a team as a code owner November 22, 2022 14:17
majagrubic
majagrubic reviewed Nov 22, 2022
View reviewed changes
packages/shared-ux/file/util/src/image_metadata.ts
canvas.width = width;
canvas.height = height;
const ctx = canvas.getContext('2d');
if (!ctx) throw new Error('Could not get 2d canvas context!');
Copy link
Contributor

@majagrubic majagrubic Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we are throwing an error here and then catching it further below and swallowing it. should we log this somewhere? or should this part be outside of the try-catch block so the error can be surfaced?

Copy link
Contributor Author

@jloleysens jloleysens Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point!

Generating a blurhash for an image is kind of a nice-to-have. If it fails for whatever reason we definitely do not want to block the user. However, swallowing this error entirely will hide cases where blurhash generation fails (for whatever reason). It'd be great to record failures somehow, perhaps to our telemetry cluster (some kind of event with a message?). Not sure logging to the console would do much good, unless users actually inform us.

Last thought: could we address this on separate PR?

majagrubic
majagrubic reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! Tested in storybook on mac os X, all looks good. Code changes are pretty straightforward. Just one question below around error catching.

@jloleysens jloleysens enabled auto-merge (squash) November 23, 2022 10:24
jloleysens and others added 2 commits November 24, 2022 12:49
@jloleysens
Merge branch 'main' into files-move-components-to-packages
86ff9af
@jloleysens
Merge branch 'main' into files-move-components-to-packages
e593b2c 
* main:
  Make page titles more consistent for Overview, Alerts, Rules, Rule Detail and Cases pages (elastic#146150)
  [Files] Delay `<Image/>` blurhash reveal and handle blurhash errors (elastic#146159)
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
files 60 63 +3
filesManagement 113 119 +6
total +9

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
@kbn/shared-ux-file-image - 2 +2
@kbn/shared-ux-file-image-mocks - 2 +2
@kbn/shared-ux-file-util - 13 +13
files 52 50 -2
total +15

Async chunks

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

id before after diff
files 26.1KB 36.3KB +10.2KB
filesManagement 80.6KB 85.5KB +4.9KB
total +15.1KB

Page load bundle

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

id before after diff
files 17.9KB 12.2KB -5.7KB
filesManagement 3.7KB 3.6KB -135.0B
total -5.9KB
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-file-image - 3 +3
@kbn/shared-ux-file-image-mocks - 2 +2
@kbn/shared-ux-file-util - 15 +15
files 294 287 -7
total +13

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
@kbn/shared-ux-file-image - 2 +2
enterpriseSearch 19 21 +2
files 4 2 -2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
total +20

Total ESLint disabled count

id before after diff
@kbn/shared-ux-file-image - 2 +2
enterpriseSearch 20 22 +2
files 8 6 -2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +21

History

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

majagrubic
majagrubic approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Changes look good. Tested in storybook in Chrome on Mac OSX

@jloleysens jloleysens merged commit 95c4d73 into elastic:main Nov 24, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 24, 2022
@jloleysens jloleysens deleted the files-move-components-to-packages branch November 24, 2022 15:58
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 28, 2022
@jloleysens
Merge branch 'main' into files-move-components-to-packages-upload-file
9133d24 
* main: (30 commits)
  [Cloud Posture] test latest findings table sort (elastic#144668)
  [api-docs] 2022-11-28 Daily api_docs build (elastic#146359)
  [api-docs] 2022-11-27 Daily api_docs build (elastic#146353)
  [api-docs] 2022-11-26 Daily api_docs build (elastic#146350)
  [DataViews] Fix form validation UX when the same data view name already exists (elastic#146126)
  [Discover] Prevent agg based visualizations of Discover saved objects with adhoc data views (elastic#145583)
  [Health Gateway] Update response aggregation (elastic#145761)
  [api-docs] 2022-11-25 Daily api_docs build (elastic#146341)
  [Metric threshold rule] Adds new context variable for group by keys (elastic#145654)
  [Controls] [Portable Dashboards] Add control group renderer example plugin (elastic#146189)
  Refactor Observability Overview Page (elastic#146182)
  Send complete test data to xMatters, so it can create an alert (elastic#145431)
  [Dashboard] [Controls] Allow options list suggestions to be sorted (elastic#144867)
  Add open API specification for list connector types (elastic#145951)
  skip flaky suite (elastic#146086)
  [ML] Removing duplicate tooltip text (elastic#146308)
  Refactor Rules Page (elastic#146193)
  [DOCS] Alert limit for cases (elastic#145950)
  Extend session index fields mapping with a session creation timestamp. (elastic#145997)
  [Files] Move <Image /> component to `@kbn/shared-ux` package (elastic#145995)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majagrubic majagrubic majagrubic approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting feature:Files release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jloleysens @kibana-ci @majagrubic @kibanamachine