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

More EuiFilePicker styles #2145

Merged
merged 10 commits into from
Jul 24, 2019
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 19, 2019

This adds some more versions of the file picker since there were a few missing.

1. Not large aka display='default' Fixes #1484

I'm not thrilled with this name, I just was having trouble finding the appropriate name. In actuality we should allow the compressed prop to accept a boolean or some string that means "normal". Suggestions welcome. UPDATED

The main reason I created this one is because I've seen consumers use the "compressed" version when they just wanted the normal height/style input. But this is technically wrong, so this is more appropriate.

Making sure the :focus / selected states works

Screen Shot 2019-07-19 at 19 45 21 PM

This also fixed an issue where truncation wasn't working.

And with multiple files

Screen Shot 2019-07-19 at 19 33 41 PM

2. fullWidth was missing

Screen Shot 2019-07-19 at 19 30 41 PM

3. isInvalid was missing

Screen Shot 2019-07-19 at 19 31 28 PM

4. Removed z-indexes

There was a clash if popovers were anywhere near:

Before
Screen Shot 2019-07-19 at 19 47 18 PM

The z-indexes weren't really necessary anyway since the input is first in DOM order.

After
Screen Shot 2019-07-19 at 19 50 47 PM

5. isLoading was missing

For the large form, I just used the animated progress indicator at the top, though I supposed we could also swap out the import icon for the loading spinner. But I do use the loading spinner for the smaller sizes. I also don't display the clear button when it's in a loading state. Simplifies the icons overload.

Screen Shot 2019-07-19 at 20 23 48 PM with a reall long name

Screen Shot 2019-07-19 at 20 31 38 PM


Height

One other big thing I changed was to set a static height on the tall version. I wasn't a fan that when selecting files it would increase in height, bumping the surrounding elements around.

Before

Since the line denoting the file name/number of files would only every be a single line, I decided to calculate the height at that moment and set it as the static height. Using flex box and column layout I'm able to keep the contents of the input vertically centered.

After


Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • [ ] This was checked for breaking changes and labeled appropriately
  • [ ] Jest tests were updated or added to match the most common scenarios
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@cchaos
Copy link
Contributor Author

cchaos commented Jul 20, 2019

I also see that @ryankeairns requested a button version of this thing, so maybe the prop isn't combined with compressed but we add the display named prop with options like 'tall' | 'normal' and eventually 'button'?

@snide
Copy link
Contributor

snide commented Jul 20, 2019

I always like using a display prop myself. Seems like it's becoming a normal pattern for us. Certainly like it more than the booleans.

@cchaos
Copy link
Contributor Author

cchaos commented Jul 20, 2019

I'll update the prop name next week tonight. But this can still be reviewed for the other parts.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled & tested locally

@cchaos
Copy link
Contributor Author

cchaos commented Jul 23, 2019

@snide This PR is ready for review.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This attacks the more immediate problem. I think it's OK to merge even if it's a bit sneaky.

* but `display: default` is not. This can be removed once
* we support the new compressed styles and make this breaking change.
*/
const calculatedDisplay = compressed ? 'default' : display;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better way to handle this. It's all because this component is so different then what we do with the rest of the form inputs where it has an alternate view that sometimes takes compressed.

Part of me wants it to be display: ['button', 'form'] and then compressed and we drop the large format, but even that wouldn't match, cuz we use size for the buttons.

I think it's the "large" part that feels the most wrong, since we normally would use that for size and would say l instead. Maybe calling it bigTarget or something?

You commented this well enough though, and even though it's a little magical, my guess is we're gonna wanna improve this component visually at some point in the future and we can worry about it then. 🛴

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't want to remove the compressed boolean option because that would cause a breaking change. I struggled with the naming of large | default as well but couldn't settle on one that felt appropriate for what it was doing which is basically just making it a larger size.

@snide Would you like me to display: ['bigTarget', 'smallTarget'] ( and button eventually)

Copy link
Contributor

Choose a reason for hiding this comment

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

No I'd rather you just merge it so that I forget that we're doing this. I think it's fine and is just some of my organization stuff is getting in the way.

I think we're gonna want to redo the visuals of this thing completely at some point, so I'm not worried about it. We can make it be more on pattern then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can always make breaking change later too. This is just a quick shim for those extra states.

cchaos added 9 commits July 24, 2019 14:22
Though I’m not keen on that name
…so that it doesn't shift it's surrounding contents around
Clashed with any popovers and wasn’t necessary since dom order signifies this
… overrides clear button
Which ended up refactoring the SASS to have the normal style control as default with `—large` overrides.
@cchaos cchaos merged commit 88106c0 into elastic:master Jul 24, 2019
@cchaos cchaos deleted the more-file-picker-styles branch July 24, 2019 21:18
thompsongl pushed a commit to thompsongl/eui that referenced this pull request Sep 10, 2019
* Added `fullWidth` and `isInvalid` props to EuiFilePicker

* Added a ‘large’ vs 'default' version

* Setting the height statically so that it doesn't shift it's surrounding contents around

* Remove z-indexes: Clashed with any popovers and wasn’t necessary since dom order signifies this

* Added isLoading state (overrides clear button)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a standard 40px height option for the EuiFilePicker
3 participants