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

[Dropdown]: Adds error slot using hasErrors and showErrors #432

Merged
merged 2 commits into from
Oct 26, 2023
Merged

[Dropdown]: Adds error slot using hasErrors and showErrors #432

merged 2 commits into from
Oct 26, 2023

Conversation

benjaminknox
Copy link
Contributor

@benjaminknox benjaminknox commented Oct 12, 2023 •
edited by fallaciousreasoning
Loading

Fixes #411

Adds error props to the drop down menu. I copied this pattern from the Input component.

Props:

  • hasErrors
  • showErrors
  • Adds a slot for a custom error message

Screenshot from storybook:
image

@benjaminknox benjaminknox mentioned this pull request Oct 12, 2023
Closed
@benjaminknox benjaminknox force-pushed the bpk/adds-error-props-to-dropdown branch from 01e87f2 to 14c3870 Compare October 12, 2023 19:48
@fallaciousreasoning
Copy link
Collaborator

Wow thanks @benjaminknox! I think you're the first external contributor to this project, congratulations.

Looks like the format is currently failing - if you run npm run format in the Leo repository and commit the result it should pass. This looks great!

@benjaminknox benjaminknox force-pushed the bpk/adds-error-props-to-dropdown branch from c038c73 to 01be7c3 Compare October 16, 2023 01:56
@benjaminknox
Copy link
Contributor Author

@fallaciousreasoning Ran the formatting command, thanks for saying something!

I think you're the first external contributor to this project, congratulations.

So cool, happy to help!

fallaciousreasoning
fallaciousreasoning reviewed Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

This looks good to me - I've just pinged our devops team because the Github Action seems to be broken (not a problem on your end).

Once it's working I'll get this merged! Thanks heaps @benjaminknox!

@benjaminknox benjaminknox force-pushed the bpk/adds-error-props-to-dropdown branch from 01be7c3 to 436c7ce Compare October 17, 2023 14:00
fallaciousreasoning
fallaciousreasoning approved these changes Oct 17, 2023
View reviewed changes
Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - I think our CI is struggling with pushes from external contributors. Hopefully me adding the merge commit is going to give it the kick it needs 😆

Merging didn't help - going to see if it works now I've touched the timestamps on the commits 😨

@fallaciousreasoning fallaciousreasoning force-pushed the bpk/adds-error-props-to-dropdown branch from 0fad5a6 to 436c7ce Compare October 17, 2023 20:02
@fallaciousreasoning fallaciousreasoning force-pushed the bpk/adds-error-props-to-dropdown branch from 436c7ce to 34982ed Compare October 17, 2023 20:03
@fallaciousreasoning fallaciousreasoning mentioned this pull request Oct 17, 2023
Closed
@fallaciousreasoning
Copy link
Collaborator

Testing to see if creating a different PR with the same commits fixes it in #441

@fallaciousreasoning fallaciousreasoning force-pushed the bpk/adds-error-props-to-dropdown branch from 33491c2 to 7d8dbfb Compare October 17, 2023 22:54
@fallaciousreasoning
Copy link
Collaborator

fallaciousreasoning commented Oct 22, 2023 •
edited
Loading

@benjaminknox a quick update now I'm back:

  1. I confirmed the build is failing because you're an external contributor (and me amending the commits doesn't help).
  2. DevOps has a solution which should fix it, and they're just confirming it's okay to break the build/deploy into separate tasks.
  3. If they end up deciding against it, I'll get someone else to approve Separate PR for #432 to try and get CI to pass #441 and we can merge your commits that way (I'd prefer not to do this because it'll mark you as a Co-Author, rather than author)

Sorry this is taking so long 😆

@benjaminknox
Copy link
Contributor Author

Sorry this is taking so long 😆

No worries! Either way is great

@benjaminknox benjaminknox force-pushed the bpk/adds-error-props-to-dropdown branch from 7d8dbfb to 4fa2f02 Compare October 25, 2023 21:45
@fallaciousreasoning fallaciousreasoning force-pushed the bpk/adds-error-props-to-dropdown branch from 4fa2f02 to 6a77695 Compare October 26, 2023 19:28
@fallaciousreasoning fallaciousreasoning merged commit d7c79e1 into brave:main Oct 26, 2023
5 checks passed
@benjaminknox benjaminknox deleted the bpk/adds-error-props-to-dropdown branch October 26, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fallaciousreasoning fallaciousreasoning fallaciousreasoning approved these changes

@petemill petemill Awaiting requested review from petemill petemill is a code owner

@AlanBreck AlanBreck Awaiting requested review from AlanBreck AlanBreck is a code owner

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

Successfully merging this pull request may close these issues.

Dropdown needs error props
2 participants
@benjaminknox @fallaciousreasoning