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

fix(Search): add disabled prop #6916

Merged
merged 9 commits into from
Oct 2, 2020
Merged

fix(Search): add disabled prop #6916

merged 9 commits into from
Oct 2, 2020

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Sep 25, 2020

Closes #6915

This PR adds an explicit disabled prop and prop type definition to the Search component and updates the corresponding stories. Even though the prop currently works as expected, we should document this prop separately and add it to the storybook

Changelog

New

  • disabled prop on Search component and stories

Testing / Reviewing

Confirm that the Search component disabled state is still applied when passing in the disabled prop, and that there are no style regressions due to missing or incorrect selectors (the styles are already in the library)

@emyarod emyarod requested a review from a team September 25, 2020 16:53
@emyarod emyarod requested a review from a team as a code owner September 25, 2020 16:53
@emyarod emyarod requested review from aagonzales, tw15egan and andreancardona and removed request for a team September 25, 2020 16:53
@netlify
Copy link

netlify bot commented Sep 25, 2020 •
edited

Deploy preview for carbon-elements ready!

Built with commit 033c992

https://deploy-preview-6916--carbon-elements.netlify.app

@emyarod emyarod force-pushed the 6915-search-component-disabled-prop branch from 8ca6014 to 1514972 Compare September 25, 2020 17:12
@emyarod emyarod requested a review from a team as a code owner September 25, 2020 17:15
@netlify
Copy link

netlify bot commented Sep 25, 2020

Deploy preview for carbon-components-react ready!

Built with commit 8ca6014

https://deploy-preview-6916--carbon-components-react.netlify.app

@tw15egan
Copy link
Member

Should the search icon use disabled token when disabled?

Screen Shot 2020-09-25 at 10 33 46 AM

@aagonzales
Copy link
Member

Yes, the icon should also be disabled-02

@netlify
Copy link

netlify bot commented Sep 25, 2020 •
edited

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 033c992

https://deploy-preview-6916--carbon-components-react.netlify.app

@emyarod
Copy link
Member Author

emyarod commented Sep 25, 2020

I don't think the component currently changes the icon color when you add the disabled prop so this isn't a regression but I can make that change here as well

@tw15egan
Copy link
Member

Looks great! Just seeing a tiny flicker of focus if you click directly on the x when it is disabled

search

tw15egan
tw15egan approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 ✅

@joshblack
Copy link
Contributor

bump @andreancardona when you get a sec today 👀

andreancardona
andreancardona approved these changes Sep 28, 2020
View reviewed changes
Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

@emyarod looks good :)

@emyarod emyarod mentioned this pull request Oct 1, 2020
Closed
50 tasks
@kodiakhq kodiakhq bot merged commit 3c107c8 into carbon-design-system:master Oct 2, 2020
@emyarod emyarod deleted the 6915-search-component-disabled-prop branch October 6, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aagonzales aagonzales aagonzales approved these changes

@tw15egan tw15egan tw15egan approved these changes

@andreancardona andreancardona andreancardona approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Search component missing disabled prop and definition
5 participants
@emyarod @tw15egan @aagonzales @joshblack @andreancardona