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

[EuiFilePicker] Use alert icon when isInvalid #6678

Merged
merged 4 commits into from
Apr 5, 2023
Merged

[EuiFilePicker] Use alert icon when isInvalid #6678

merged 4 commits into from
Apr 5, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Apr 4, 2023 •
edited
Loading

Summary

As part of my on-call work this week, I'm finishing up some older backlog work, in this case #2017.

Before

After

Non-large:

QA

General checklist

  • Checked in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • A changelog entry exists and is marked appropriately

- [ ] Added or updated jest and cypress tests There aren't really any for EuiFilePicker. We should remedy this when we switch to Storybook

  • Updated the Figma library counterpart - I need to do this later once I figure out Figma.

@cee-chen cee-chen changed the title [EuiFilePicker] Add alert icon for isInvalid state [EuiFilePicker] Add alert icon when isInvalid Apr 4, 2023
@cee-chen cee-chen changed the title [EuiFilePicker] Add alert icon when isInvalid [EuiFilePicker] Use alert icon when isInvalid Apr 4, 2023
@daveyholler
Copy link
Contributor

I'm gonna be a little pedantic… Does it make more sense here to use an error icon with the danger color?

Just thinking about the guidelines we gave to folks in that PR from a couple weeks ago that the alert icon should be used when when the alert can be safely ignored, and the error icon should be used when alert is blocking. And also how those 2 icons map to respective colors in the palette.

Though maybe that error icon would just feel out of place here?

@MichaelMarcialis tagging you on this question as well. I'll roll with what y'all think is best.

@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

cee-chen commented Apr 4, 2023 •
edited
Loading

Every other form icon on https://elastic.github.io/eui/#/forms/form-controls uses the alert icon for isInvalid state - I would be against changing this as a one-off for EuiFilePicker. If we want to evaluate every single component at once to use error instead of alert, we should open a separate issue for that.

@kibanamachine
Copy link

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

@cee-chen cee-chen requested review from JasonStoltz and breehall and removed request for daveyholler April 4, 2023 16:22
@cee-chen
Copy link
Member Author

cee-chen commented Apr 5, 2023

@breehall do you mind reviewing this PR sometime today? Thanks!

breehall
breehall approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

✅ Approved. I used different combinations of props within the Display Toggles for EuiFilePicker in the PR preview to confirm these changes.

@cee-chen cee-chen merged commit 34748f9 into elastic:main Apr 5, 2023
@cee-chen cee-chen deleted the filepicker/invalid branch April 5, 2023 16:40
@breehall breehall mentioned this pull request Apr 12, 2023
Merged
jbudz added a commit to elastic/kibana that referenced this pull request Apr 18, 2023
@breehall @kibanamachine @jbudz
Upgrade EUI to v77.1.1 (#154838)
3259647 
EUI `77.0.0` ➡️ `77.1.1`

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

- Updated `EuiDatePicker` to display a warning icon and correctly set
`aria-invalid` when `isInvalid` is passed
([#6677](elastic/eui#6677))
- Updated `EuiFilePicker` to display an alert icon when `isInvalid`
([#6678](elastic/eui#6678))
- Updated `EuiTextArea` to display an alert icon when `isInvalid`
([#6679](elastic/eui#6679))
- Updated `EuiTextArea` to support the `isLoading` prop
([#6679](elastic/eui#6679))
- Updated `EuiComboBox` to display a warning icon and correctly set
`aria-invalid` when `isInvalid` is passed
([#6680](elastic/eui#6680))

**Bug fixes**

- Fixed `EuiAccordion` to not set an `aria-expanded` attribute on
non-interactive `buttonElement`s
([#6694](elastic/eui#6694))
- Fixed an `EuiPopoverFooter` bug causing nested popovers within
popovers (note: not a recommended use-case) to unintentionally override
its panel padding size inherited from context
([#6698](elastic/eui#6698))
- Fixed `EuiComboBox` to only delete the last selected item on backspace
if the input caret is present
([#6699](elastic/eui#6699))

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Jon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breehall breehall breehall approved these changes

@JasonStoltz JasonStoltz Awaiting requested review from JasonStoltz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @daveyholler @kibanamachine @breehall