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 feedback prop to input to support dynamic styling #1196

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Aug 16, 2023

This PR adds a new feedback prop to Input, that can have values error or warning, and is used to dynamically style the input to provide extra feedback.

Note
I named it feedback, after checking how other UI libs treat this. Feedback is how it's named in bootstrap, and it sounds intuitive enough, and allows for more values to be eventually added (success, info, etc)

image

It also deprecates and replaces hasError, recommending feedback"error" as a direct replacement.

image

Note
The Select was left out of this PR and will be added afterwards, once we have discussed and agreed on the right contract.

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #1196 (399e79f) into main (a4d6143) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 399e79f differs from pull request most recent head 011e61a. Consider uploading reports for the commit 011e61a to get more accurate results

@@            Coverage Diff            @@
##              main     #1196   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           57        57           
  Lines          806       807    +1     
  Branches       312       313    +1     
=========================================
+ Hits           806       807    +1     
Files Changed Coverage Δ
src/components/input/Input.tsx 100.00% <ø> (ø)
src/components/input/InputRoot.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

{
'ring-inset ring-2': !!feedback || hasError,
'ring-red-error': feedback === 'error' || hasError,
'ring-yellow-notice': feedback === 'warning',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yellow-notice is the same color we use for notice toast messages.

@acelaya acelaya requested a review from robertknight August 17, 2023 11:16
@acelaya acelaya mentioned this pull request Aug 21, 2023
@acelaya acelaya marked this pull request as ready for review August 21, 2023 14:56
@@ -27,6 +32,7 @@ const InputRoot = function InputRoot({
classes,
elementRef,
hasError,
feedback,
Copy link
Member

Choose a reason for hiding this comment

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

Since hasError is deprecated and maps directly to a value of feedback, I would suggest that you do the mapping at the top of the component and avoid referencing hasError later. This will make the mapping more obvious and simplify removing hasError later on.

@robertknight
Copy link
Member

I named it feedback, after checking how other UI libs treat this.

The name sounds reasonable. Is Bootstrap the only library you looked at or were there others?

@acelaya
Copy link
Contributor Author

acelaya commented Aug 23, 2023

The name sounds reasonable. Is Bootstrap the only library you looked at or were there others?

I also checked Material UI, where they call it just color, accepting values warning, success, etc. Although, they combine it with a boolean error prop which turns the input "red".

I thought feedback was more concise than color, and also prefer to have just one way to do something.

@acelaya acelaya force-pushed the input-feedback branch 2 times, most recently from d0beae4 to 399e79f Compare August 23, 2023 14:07
@acelaya acelaya requested a review from robertknight August 23, 2023 14:08
@robertknight
Copy link
Member

I thought feedback was more concise than color, and also prefer to have just one way to do something.

If we only support displaying one feedback state at a time, then indeed a single prop seems a better way to model this.

src/components/input/InputRoot.tsx Outdated Show resolved Hide resolved
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.

2 participants