[Files] Filepicker #143111

jloleysens · 2022-10-11T16:38:14Z

Adds the new Filepicker component (modal). See the resulting files storybooks to test it out.

Notes to reviewers

~~This component does not use "true" pagination at the moment because the list endpoint does not support filtering, we should add this in a follow up PR~~
To make uploads easier we should layer on a component that makes the entire modal a droppable region that will upload files, right now it is only possible to upload a file when the component has no files loaded
If you want to get the best experience of the component in action (i.e., actually talking to Elastic) I would boot up Kibana with examples running yarn start --run-examples and open the filesExample plugin (at app/filesExample). Upload a few documents via the "Upload" button and then pick 'em in "Picker"!

Screenshots

… adding of duplicates

…ve on upload controls

…n texts for empty state prompt

jloleysens · 2022-10-19T12:54:29Z

I don't see an "Upload" button or some other way to bring up the upload component, once there are some files already uploaded:

I'll address this in a follow up PR. We can probably just add the compact version of the filepicker somewhere.

jloleysens · 2022-10-19T14:35:02Z

@elasticmachine merge upstream

mdefazio

A few comments:

Uploading a file

Should the text inside the file upload simply say 'Select or drop a file' to be more concise? Removes the extra conjunction.

When uploading the file, let's use the [loading button](https://elastic.github.io/eui/#/navigation/button#loading-state) , and then change it to a `success` button with the check icon. The label could then say 'Upload complete'. We would not need to use the pink line loading indicator in this case then. I would imagine this would be the same behavior for the `Basic` and `Allow repeated uploads` options.

The Long error scenario should also use the default button, instead of an empty button. Though in this case, we could use the danger styling. The error messages seem fine though.

If I abort the upload, should that message not appear somewhere? Also switching this to a default button rather than empty like the other scenarios can help keep this upload button the main focus. Perhaps reusing the Max size option here, but simply changing the text to 'File upload cancelled'

The Allow clear after upload should probably be colored as danger and say 'Remove file', since this is what we use within the file upload box as well. Perhaps also including the crossInCircleFilled icon here keeps this consistent with our clearing an input box.

The Immediate upload can use the same success message as the previous scenarios, rather than only an icon.

Same feedback for Immediate upload error.
The Immediate upload abort Retry should show a default button as mentioned in previous scenarios.

File picker

Not sure if this is part of the component, but if the button says Pick a file, the modal title should likely say 'Pick a file' rather than 'Select a file'. I would vote for 'Select a file' as the button label, but I believe these two should be consistent (as well as the save/confirmation button).

The Empty version has 'No files found' and 'Upload your first file'. These seem to contradict each other. Is this also reused as the empty state for searching? Maybe we don't need to include 'Upload your first file' if so?

The Error loading screen has the word 'stop' as the description text. Is this necessary? Or can we change it something more meaningful?

Im obviously reviewing the storybook examples, but will run this locally shortly as well. Please reach out if these comments don't make sense. This is really nice overall!

Update - File upload screen

Note sure if these are necessary for the component...but here goes:

I would also suggest using the left-aligned, default (stacked) description list, rather than reversing these and using columns.
I think the close button should be on the left side of the flyout, with 'Download' being on the right.
What are the possibilities for the Status column? Do we need this?
Wondering if we need to use the eye icon here. And just let them click on the file name?
And I would use our typical Confirm modal dialog when deleting a file. Unless I'm missing this pattern being adopted elsewhere? Perhaps the delete icon doesn't need to always be visible, but only show on row hover?

jloleysens · 2022-10-20T08:22:01Z

@mdefazio

Uploading a file

Thanks for the design feedback on the upload file component! It was very much needed. This component is not being introduced in this PR, but I'll make an issue of the comments you left and address them in a separate PR! I've opened this issue with your comments: #143724

jloleysens · 2022-10-20T08:29:05Z

Not sure if this is part of the component, but if the button says Pick a file, the modal title should likely say 'Pick a file'...

At the moment the modal's title is "Select a file" and the button says "Select file". When the user selects more than one file the text on the button changes to "Select x files" where x is the number of selected files.

This does still need copy review, I'll be sure to get it!

The Empty version has 'No files found' and 'Upload your first file'. These seem to contradict each other...

Good point, this also needs copy review. I was trying to use the empty to prompt the user to action. The user should only see this state if they have no files uploaded. This "no data" view will probably not be seen by most users.

The Error loading screen has the word 'stop' as the description text

😄 the text "stop" is from the error I was throwing in the story book. Normally, this would contain the error of the network request. Telling users "stop!" would be pretty funny in this context though!

mdefazio · 2022-10-20T13:32:15Z

At the moment the modal's title is "Select a file" and the button says "Select file". When the user selects more than one file the text on the button changes to "Select x files" where x is the number of selected files.

This was regarding the button that launches the modal...shown in the PR description.

Again, didn't know if this was part of the component or up to the developer using it. But would be good to add a note that these should match, or a recommended label.

The user should only see this state if they have no files uploaded. This "no data" view will probably not be seen by most users.

What if they search and return no results? I agree pushing an action on the empty state. Perhaps just removing the word 'first' would avoid any potential confusion?

Normally, this would contain the error of the network request.

So not something this component controls? Do we have a set of common network errors that we provide stock copy for?

Thanks for bearing with me on the follow up questions. Just wanted to clarify and then I'll ✅

jloleysens · 2022-10-20T14:33:23Z

Again, didn't know if this was part of the component or up to the developer using it. But would be good to add a note that these should match, or a recommended label.

I see, apologies for the misunderstanding. This is the developer examples app, shouldn't ever be seen by non-devs, but happy to go with your recommendation!

What if they search and return no results? I agree pushing an action on the empty state. Perhaps just removing the word 'first' would avoid any potential confusion?

Ah I see, in that case the user will see "No files matched filter".

So not something this component controls? Do we have a set of common network errors that we provide stock copy for?

Right, not something this component controls. This is intended as a catch-all message where the summary is "We couldn't load the files". The action will always be to just retry. If that doesn't work, they have an error message that might help or that they can report to us if it comes to it.

mdefazio · 2022-10-20T15:34:45Z

x-pack/plugins/files/public/components/file_picker/components/file_card.scss

@@ -0,0 +1,5 @@
+.filesFilePicker {
+  .euiCard__content, .euiCard__description {
+    margin :0; // make the cards a little bit more compact


Not that I don't agree with you. But I'm not sure there's a significant reason to overwrite our EUI styles here. Ideally we wouldn't need to do this to save future maintenance (and work for you)

mdefazio

Just a comment on the margin override. Not a huge deal, thanks for setting up the other issue with my comments 🙇

jloleysens · 2022-10-22T22:54:25Z

@elasticmachine merge upstream

jloleysens · 2022-10-24T07:44:02Z

@elasticmachine merge upstream

kibana-ci · 2022-10-24T08:46:42Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`files`	41	68	+27

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
`files`	14	18	+4

Async chunks

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

id	before	after	diff
`files`	9.0KB	25.3KB	+16.4KB

Page load bundle

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

id	before	after	diff
`files`	18.9KB	19.5KB	+573.0B

Unknown metric groups

API count

id	before	after	diff
`files`	260	271	+11

async chunk count

id	before	after	diff
`files`	1	2	+1

ESLint disabled in files

id	before	after	diff
`files`	3	4	+1

Total ESLint disabled count

id	before	after	diff
`files`	6	7	+1

History

💛 Build #81995 was flaky 75adddf
💛 Build #81807 was flaky 151ead5
💚 Build #81433 succeeded 8946c03
💔 Build #81372 failed 7b83fe1
💚 Build #81019 succeeded 3276146
💔 Build #80997 failed 25b04ac

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

* main: (57 commits) [Files] Filepicker (elastic#143111) [Infrastructure UI] Replace Lens table with EUI table and own api (elastic#142871) [api-docs] Daily api_docs build (elastic#143829) [api-docs] Daily api_docs build (elastic#143825) [api-docs] Daily api_docs build (elastic#143823) [Security Solution] Restructuring folders of Detection Engine + refactoring Rule Management (elastic#142950) [Dev tools] Fix performance issue with autocomplete suggestions (elastic#143428) [Security Solution] Disable ML rule's edit button link under basic license (elastic#143260) [Lens] Use the language-documentation package for formula (elastic#143649) [api-docs] Daily api_docs build (elastic#143811) [Security Solution] Fix missing title on inspect pop-up (elastic#143601) fix incorrect filters being passed to events table causing duplicate entries in our inpsect tool request tab (elastic#143239) [Security Solution][Endpoint] `get-file` response action kibana download file API (elastic#143708) Rely on refresh context to update stats independently of overview cards. (elastic#143308) [RAM] Rule event log - Fix incorrect results when filtering by message and outcome simultaneously (elastic#143119) [ML] Display link to create data view from error cases in data frame analytics results pages (elastic#143596) Update links in README :) (elastic#143675) Add more tests for ml_inference_logic (elastic#143764) skip failing test suite (elastic#143717) [DOCS] Add assignees to case APIs (elastic#143610) ...

gchaps · 2022-10-26T22:55:13Z

x-pack/plugins/files/public/components/file_picker/i18n_texts.ts

+    defaultMessage: 'my-file-*',
+  }),
+  emptyFileGridPrompt: i18n.translate('xpack.files.filePicker.emptyGridPrompt', {
+    defaultMessage: 'No files matched filter',


No files match your filter

gchaps · 2022-10-26T23:15:06Z

x-pack/plugins/files/public/components/file_picker/i18n_texts.ts

+
+export const i18nTexts = {
+  title: i18n.translate('xpack.files.filePicker.title', {
+    defaultMessage: 'Select a file',


The title is good if you can select 1 file. If you can select multiple files, then the title should be "Select files",

gchaps · 2022-10-26T23:15:20Z

x-pack/plugins/files/public/components/file_picker/i18n_texts.ts

+    defaultMessage: 'Select a file',
+  }),
+  loadingFilesErrorTitle: i18n.translate('xpack.files.filePicker.error.loadingTitle', {
+    defaultMessage: 'Could not load files',


The title is "Select a file" (singular) but the message is "Could not load files" (plural). This should match.

Also, suggest this wording "Cannot load files"

The body of the message says "stop". Can you give a reason as to why the files cannot be loaded?

gchaps · 2022-10-26T23:15:28Z

x-pack/plugins/files/public/components/file_picker/i18n_texts.ts

+  retryButtonLabel: i18n.translate('xpack.files.filePicker.error.retryButtonLabel', {
+    defaultMessage: 'Retry',
+  }),
+  emptyStatePrompt: i18n.translate('xpack.files.filePicker.emptyStatePrompt', {


Is this an empty state? If so, can "Upload your first file" be the heading?

jloleysens added 30 commits October 10, 2022 12:26

added unscoped file client to the files components context

f184096

wip: created some basic files and stories for new filepicker component

ddf2173

fix some types after require filesclient to be passed in

41ed423

added file picker state and some basic tests

a85a1dc

added file picker context

2661683

added missing file client value

a2221d9

updated file picker stories

a129c4b

expanded files context;

42459a2

remove the size observable and also added a test for file removal and…

dfbaae5

… adding of duplicates

added error content component

1538c92

refactor upload component to not take files client as prop

70d7495

updated shared file compoennts context value

5f61346

added test for adding multiple files at once

0dd866d

allow passing in string or array of strings

1a3b5f7

added some i18n texts

c87a239

move the creation of a file kinds registry to a common location

4c9f438

set file kinds only once

f080aec

a bunch of stuff: added title component, using grid, removed responsi…

98fa226

…ve on upload controls

refactor layour components to own component using grid, added new i18…

8391853

…n texts for empty state prompt

minor copy tweak

569de31

added basic story with some files

3a49db2

added file grid and refactored picker to only exist in modal for now

6844647

get the css grid algo that we want: auto-fill not auto-fit!

f2e71a5

override styling for content area of file

69ba34b

split stories of files

abaf1f2

delete commented out code

51ba099

give the modal a fixed width

9bd9f75

fix upload file state, where we do not want a fixed width modal

e56b10a

moved styles down to card, and combined margin removal rules

92ca412

optimize for filtering files, first pass just filter on names

1b8c4d6

Merge branch 'main' into files-filepicker-KBNA2079

8946c03

mdefazio requested changes Oct 19, 2022

View reviewed changes

jloleysens mentioned this pull request Oct 20, 2022

[Files] Design feedback for the upload component #143724

Closed

jloleysens closed this Oct 20, 2022

auto-merge was automatically disabled October 20, 2022 08:22
Pull request was closed

jloleysens reopened this Oct 20, 2022

copy update in files example app

784cf1c

added filter story

151ead5

mdefazio reviewed Oct 20, 2022

View reviewed changes

mdefazio approved these changes Oct 20, 2022

View reviewed changes

Merge branch 'main' into files-filepicker-KBNA2079

75adddf

Merge branch 'main' into files-filepicker-KBNA2079

c560b17

jloleysens merged commit a42f182 into elastic:main Oct 24, 2022

kibanamachine added the backport:skip This commit does not require backporting label Oct 24, 2022

jloleysens deleted the files-filepicker-KBNA2079 branch October 24, 2022 09:00

This was referenced Oct 24, 2022

[Files] Fix image intersection observer behavior #143843

Merged

[Files] Add file upload to file picker #143969

Merged

gchaps reviewed Oct 26, 2022

View reviewed changes

jloleysens mentioned this pull request Nov 2, 2022

[Files] Copy updates and file picker single select #144398

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Files] Filepicker #143111

[Files] Filepicker #143111

jloleysens commented Oct 11, 2022 •

edited

Loading

jloleysens commented Oct 19, 2022

jloleysens commented Oct 19, 2022

mdefazio left a comment •

edited

Loading

jloleysens commented Oct 20, 2022 •

edited

Loading

jloleysens commented Oct 20, 2022

mdefazio commented Oct 20, 2022

jloleysens commented Oct 20, 2022 •

edited

Loading

mdefazio Oct 20, 2022

mdefazio left a comment

jloleysens commented Oct 22, 2022

jloleysens commented Oct 24, 2022

kibana-ci commented Oct 24, 2022

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

gchaps Oct 26, 2022

gchaps Oct 26, 2022

gchaps Oct 26, 2022

gchaps Oct 26, 2022

[Files] Filepicker #143111

[Files] Filepicker #143111

Conversation

jloleysens commented Oct 11, 2022 • edited Loading

Notes to reviewers

Screenshots

jloleysens commented Oct 19, 2022

jloleysens commented Oct 19, 2022

mdefazio left a comment • edited Loading

Choose a reason for hiding this comment

Uploading a file

File picker

jloleysens commented Oct 20, 2022 • edited Loading

jloleysens commented Oct 20, 2022

mdefazio commented Oct 20, 2022

jloleysens commented Oct 20, 2022 • edited Loading

mdefazio Oct 20, 2022

Choose a reason for hiding this comment

mdefazio left a comment

Choose a reason for hiding this comment

jloleysens commented Oct 22, 2022

jloleysens commented Oct 24, 2022

kibana-ci commented Oct 24, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled in files

Total ESLint disabled count

History

gchaps Oct 26, 2022

Choose a reason for hiding this comment

gchaps Oct 26, 2022

Choose a reason for hiding this comment

gchaps Oct 26, 2022

Choose a reason for hiding this comment

gchaps Oct 26, 2022

Choose a reason for hiding this comment

jloleysens commented Oct 11, 2022 •

edited

Loading

mdefazio left a comment •

edited

Loading

jloleysens commented Oct 20, 2022 •

edited

Loading

jloleysens commented Oct 20, 2022 •

edited

Loading