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

EuiFilePicker dropzone style updates #6479

Merged
merged 21 commits into from
Dec 14, 2022
Merged

Conversation

daveyholler
Copy link
Contributor

@daveyholler daveyholler commented Dec 13, 2022

Summary

This PR updates EuiFilePicker dropzone styles to look more like an interactive element. Text contrast is boosted in the compact variant of the component and it no longer looks like a disabled button or inactive input.

image

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@daveyholler daveyholler requested a review from cee-chen December 13, 2022 18:21
@cee-chen
Copy link
Contributor

cee-chen commented Dec 13, 2022

Two notes separate from the code changes:

Updated the Figma library counterpart

  1. Do you have access to Figma Davey? Can you update the designs there?

and

  1. Do you mind updating the PR title to something a bit more descriptive, as that's what GitHub uses for the commit message when it gets squash merged? Something like [EuiFilePicker] Update styles would be fine

@daveyholler daveyholler changed the title Dropzone EuiFilePicker dropzone style updates Dec 13, 2022
@daveyholler
Copy link
Contributor Author

@constancecchen - Title updated (I'm trying to do too many things at once)... And yes, I'll be updating Figma.

@daveyholler daveyholler requested a review from cee-chen December 13, 2022 18:39
@daveyholler daveyholler enabled auto-merge (squash) December 13, 2022 18:40
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6479/

@cee-chen
Copy link
Contributor

Sorry, only just opened this up to QA - it looks like both the disabled and compressed styling has a box-shadow "border" that needs to be removed:

Invalid also looks very off, for lack of better word:

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6479/

@daveyholler
Copy link
Contributor Author

@constancecchen compressed, invalid, and disabled styles updated.

image

image

image

@daveyholler
Copy link
Contributor Author

The invalid styles kinda bug me, but they match our patterns elsewhere 🤷🏼‍♂️

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6479/

@cee-chen
Copy link
Contributor

🙈 I was doing one final QA pass on FF/Edge/Safari and noticed that the focus state in FF+Chrome also needs updating:

You can repro by clicking the filepicker and canceling out of the dialog

@daveyholler
Copy link
Contributor Author

@constancecchen so I addressed the focus state in Chrome and Firefox, but focus doesn't appear to render at all in Safari. However, that's also the case in the production site. Are we cool with that?

... I'm becoming painfully aware of how out of touch I am with CSS right now.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6479/

@cee-chen
Copy link
Contributor

Safari does not show focus state on click on prod either, so you're fine. It does properly show focus state on keyboard tab, and I did check and that's displaying correctly - you should be all good.

@daveyholler daveyholler requested a review from cee-chen December 14, 2022 16:49
@cee-chen
Copy link
Contributor

cee-chen commented Dec 14, 2022

transition:
box-shadow $euiAnimSpeedFast ease-in,
background-color $euiAnimSpeedFast ease-in,
background-image $euiAnimSpeedFast ease-in,
background-size $euiAnimSpeedFast ease-in $euiAnimSpeedFast; /* 3 */

This transition CSS is no longer applicable since we're setting border color instead of box-shadow. I strongly recommend changing line 81 to border-color $euiAnimSpeedFast ease-in, instead - it makes the transition from invalid to valid and disabled/non-disabled slightly smoother.

EDIT: I would remove the background-image and background-size transitions as well as the the below comment:

* 3. Delay focus gradient or else it will only partially transition while file chooser opens

AFAICT there's no background gradient on focus, this must be referring to some old styles that no longer exists - we should just remove the comment entirely.

So TL;DR of changes I'm asking for (plus removing line 67):

  transition:
    box-shadow $euiAnimSpeedFast ease-in,
    background-color $euiAnimSpeedFast ease-in;

@daveyholler daveyholler requested a review from cee-chen December 14, 2022 17:25
@daveyholler daveyholler requested a review from cee-chen December 14, 2022 17:39
@cee-chen
Copy link
Contributor

Changes look good! Will wait for staging to deploy in 30 mins or so and do another quick cross-browser QA pass, and (fingers crossed) if nothing comes up, I think this PR is good to go!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6479/

@cee-chen
Copy link
Contributor

As a heads up, I did find one extremely minor bug that transitions don't work on compressed styling, because of the below CSS property:

However, this is coming from our form mixins which is coming in from our compressed mixin:

@supports (-moz-appearance: none) {
// List *must* be in the same order as the above.
transition-property: box-shadow, background-image, background-size;
}

So I don't think this is worth solving in this PR - we should address form mixin shenanigans generally in our Emotion conversion instead.

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 Thanks for your patience - I love the new look. Let's ship it!

@daveyholler daveyholler merged commit 8d79c7d into elastic:main Dec 14, 2022
jbudz pushed a commit to elastic/kibana that referenced this pull request Dec 22, 2022
## Summary

`[email protected]` ⏩ `[email protected]`

---

## [`72.0.0`](https://github.com/elastic/eui/tree/v72.0.0)

- Added the `customQuickSelectRender` render prop to
`EuiSuperDatePicker`, which allows customizing the Quick Select popover
([#6382](elastic/eui#6382))
- `EuiFilePicker` styles have been updated to look more like an
interactive element. ([#6479](elastic/eui#6479))
- Added a third argument to `EuiSelectable`'s `onChange` callback. The
single `option` object that triggered the `onChange` event is now also
passed to consumers with its most recent `checked` state
([#6487](elastic/eui#6487))

**Bug fixes**

- `EuiTabs` now passes `size` and `expand` to all children using a React
context provider. ([#6478](elastic/eui#6478))
- Fixed security warnings caused by `[email protected]` sub-dependency
([#6482](elastic/eui#6482))

**Breaking changes**

- Removed `size` and `expand` props from `EuiTab`
([#6478](elastic/eui#6478))

## [`71.1.0`](https://github.com/elastic/eui/tree/v71.1.0)

**Deprecations**

- Renamed `EuiPageSideBarProps` to `EuiPageSideBarProps_Deprecated`, to
reduce usage/confusion with `EuiPageSidebar`
([#6468](elastic/eui#6468))

Co-authored-by: Kibana Machine <[email protected]>
@cee-chen cee-chen mentioned this pull request Jun 13, 2024
27 tasks
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.

3 participants