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

Add FilePicker component #1537

Merged
merged 16 commits into from
Jan 20, 2023
Merged

Add FilePicker component #1537

merged 16 commits into from
Jan 20, 2023

Conversation

bytasv
Copy link
Contributor

@bytasv bytasv commented Jan 9, 2023

CleanShot.2023-01-10.at.13.55.02.mp4

@bytasv bytasv self-assigned this Jan 9, 2023
@oliviertassinari oliviertassinari requested a deployment to file-picker - toolpad-db PR #1537 January 9, 2023 14:12 — with Render Abandoned
Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Signed-off-by: Vytautas Butkus <vytautas.butkus@gmail.com>
@oliviertassinari oliviertassinari temporarily deployed to file-picker - toolpad PR #1537 January 10, 2023 12:44 — with Render Destroyed
@oliviertassinari oliviertassinari temporarily deployed to file-picker - toolpad PR #1537 January 10, 2023 12:46 — with Render Destroyed
@bytasv bytasv requested a review from Janpot January 10, 2023 12:46
@Janpot
Copy link
Member

Janpot commented Jan 10, 2023

Can you also add helperText properties to the component and to each arg type definition. See the other components for an example.

Also, should we put a bit of effort in making this look a bit better? It feels a bit out of placed compared to the MUI components. Perhaps start by making a benchmark of Material Design (visually) compatible Filepicker components that have been created in the wild?

@oliviertassinari oliviertassinari temporarily deployed to file-picker - toolpad PR #1537 January 11, 2023 06:20 — with Render Destroyed
@bytasv
Copy link
Contributor Author

bytasv commented Jan 11, 2023

@Janpot helper text added

Also, should we put a bit of effort in making this look a bit better? It feels a bit out of placed compared to the MUI components. Perhaps start by making a benchmark of Material Design (visually) compatible Filepicker components that have been created in the wild?

Part of this PR - I don't think so. I don't see this being a blocker for the functionality. It does however inherit props from TextField so it isn't entirely "out of place".
However I do agree that it should look better, though question is whether we should do a special implementation for toolpad only or cooperate with core and finally have file picker component in core library?

-edit: alternatively I could try to use a Button component so it looks like a button to select files and not like an input

@bytasv
Copy link
Contributor Author

bytasv commented Jan 17, 2023

Right in canvas? I'd leave that to the end user to decide, by using other components

Yes, that would be just like the native file input, so I don't think it's an unreasonable functionality. We're a low-code tool for internal applications, so our default functionality should cover the majority of use-cases.

Is the suggestion to hardcode icon along side the content?

The user should get a good default experience. We can allow overriding behavior, but the default should look good and cover the majority of use-cases.

Can you define exactly what you want to see?

I had a default file input behaviour implemented which was just changed to the button, it did show file name out of the box, it also shown if multiple files were selected.

At this point it's not really clear how it should look like.

@Janpot
Copy link
Member

Janpot commented Jan 17, 2023

At this point it's not really clear how it should look like.

Is it possible to make it look just like the native file input, but with an MUI Button (that has an icon for extra visual feedback)?

@bytasv
Copy link
Contributor Author

bytasv commented Jan 17, 2023

Of course it's possible, my question is just, what's the value in current stage of "over optimising" this component so much? If we want default behaviour then why not stick with previous implementation of textfield?

@joserodolfofreitas
Copy link
Member

Would something like this make sense for X at some point?

Yes. If you're willing to get started with it, it'd be great. We can incorporate it into MUI X later.

We have an issue on our repository. And we had a couple of requests for a FilePicker/ Dropzone component in the 2022 survey.
However, at the moment, and with the current bandwidth, it's unlikely that we can undertake this task in 2023.
The initial plan is to expand the components portfolio with charts and maybe advanced TreeView this year.

@Janpot
Copy link
Member

Janpot commented Jan 17, 2023

what's the value in current stage of "over optimising" this component so much?

I don't think we're over optimizing here, any PR comment can be dismissed by using the term "over optimizing". The idea was to visually tie in this component with the rest of the components, ideally without sacrificing too much functionality. I feel like a visual feedback of whether and which files are selected is an essential feature of this component.

@joserodolfofreitas Thanks, I completely missed that ticket. Been searching on "File input" and "File picker" and "Filepicker" but it doesn't seem to appear on the first few pages. (Makes me think we could introduce a keywords section in the issue template, but that's besides the question). We'll create a basic implementation of a FileInput for toolpad, and we can promote to X when there's more demand.

@bytasv
Copy link
Contributor Author

bytasv commented Jan 17, 2023

I don't think we're over optimizing here

And I think differently. I already had a version done which shows by default file name that's selected and uses default MUI TextField styles.

I can revert changes to that or keep this implementation that uses Button style. What matters is that actual file pick/upload works, and without an actual design or benchmark I don't see the point in optimising this without any external feedback. This can be further improved/optimised if you (or we as entire team) see the benefit.

any PR comment can be dismissed by using the term "over optimizing"

Any PR that CAN be dismissed by any reasonable argument should be dismissed, this way we can save time and focus on things that matter and that have positive impact.

So in order to stop bikeshedding - which of the 2 options you want me to keep?

@Janpot
Copy link
Member

Janpot commented Jan 17, 2023

There are two different concerns in tension here:

  1. We'd like to adhere mostly to a single visual language across our components so that our users can create coherent UIs
  2. A component should visually represent the state it is in. e.g. a text input or select shows the actual value, even when it's not being interacted with.
  • The default file input gives us 2. but not 1.
  • The Button implementation gives us 1. but not 2.
  • My proposal would be to change the implementation and accompany that Button with a Typography containing the names of the selected files to give us 1. and 2.

Personally I think both of these concerns are valid and aren't necessarily "bikeshedding", But I understand that this would be some extra work, so if I really had to choose, I'd say to use the default file input. It has the better UX for the user, and is easier to improve for us afterwards.

Before we merge, if you're up for it, I'd propose to add a basic test, like we have for all the other components.

@bytasv
Copy link
Contributor Author

bytasv commented Jan 17, 2023

This is how the 1st option looked like pretty much:

https://codesandbox.io/s/red-water-uqq1po?file=/demo.tsx

CleanShot 2023-01-17 at 19 49 08@2x

So saying that "it does not adhere to single visual language" is a bit of exaggeration, don't you think? While I agree it can be further OPTIMISED by getting rid of default browser button, I don't see value at this point as per comments explained before - there is no design (which means its likely going to change anyway) and we haven't done any benchmark (which means its likely to change anyway).

Before we merge, if you're up for it, I'd propose to add a basic test, like we have for all the other components.

Sure I will try to add tests only if playwright allows working with file inputs 🤔

@Janpot
Copy link
Member

Janpot commented Jan 18, 2023

So saying that "it does not adhere to single visual language" is a bit of exaggeration, don't you think?

👍 Ok, I see now where our misunderstanding originates from. For me, having a browser native button nested inside of a material design textfield would be the definition of "it does not adhere to single visual language". But I understand that the threshold is interpreted differently by different people.

@bytasv
Copy link
Contributor Author

bytasv commented Jan 18, 2023

reverted to textfield version and added test, please review

@Janpot
Copy link
Member

Janpot commented Jan 19, 2023

There's one thought I had which may be a bit of a nit, but is related to updating this to a proper FilePicker component in the future. I can imagine that new component won't have MUI TextField specific properties like variant, size or fullWidth. It could be tricky to migrate if we let users build apps with these properties and have to upgrade this component to a version that doesn't have them. Would it make sense to omit those properties for now, so that the next programmer working on this code has an easier job?

@oliviertassinari oliviertassinari temporarily deployed to file-picker - toolpad PR #1537 January 20, 2023 07:26 — with Render Destroyed
@bytasv
Copy link
Contributor Author

bytasv commented Jan 20, 2023

Props removed

@oliviertassinari oliviertassinari temporarily deployed to file-picker - toolpad PR #1537 January 20, 2023 09:54 — with Render Destroyed
@bytasv bytasv merged commit 6dd63e7 into master Jan 20, 2023
@bytasv bytasv deleted the file-picker branch January 20, 2023 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support FileInput Component
5 participants