Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

fix(Select): match experimental spec #2030

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Mar 15, 2019

Closes IBM/carbon-components-react#2026

Closes https://github.com/IBM/carbon-components-react/issues/2037

This PR incorporates feedback from the February 2019 select component audit

Depends on carbon-design-system/carbon#2114 and carbon-design-system/carbon#2135

@netlify
Copy link

netlify bot commented Mar 15, 2019

Deploy preview for carbon-components-react ready!

Built with commit 6503cf2

https://deploy-preview-2030--carbon-components-react.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM basically, just one thing - Thanks @emyarod!

<label htmlFor={id} className={labelClasses}>
{labelText}
</label>
{componentsX && !inline && helper}
Copy link
Contributor

Choose a reason for hiding this comment

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

The markup change seems to affect v9. Could you please double-check v9 is not broken?

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be fixed now @asudoh

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thank you for adding that back! Actually v9 topic came up as the helper used to be next icon whereas it's next to <label> in this PR. Wanted to make sure such move-around doesn't negatively affecting v9. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@emyarod You are right, I may have been seeing a stale commit, my apologies!

@emyarod emyarod force-pushed the 2026-match-select-to-experimental-spec branch 2 times, most recently from 72d0ab5 to 9e966e9 Compare March 19, 2019 18:46
Copy link
Collaborator

@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.

👍

@emyarod
Copy link
Member Author

emyarod commented Mar 19, 2019

currently in the middle of incorporating changes from carbon-design-system/carbon#2114 to add legacy support for inline helper text @tw15egan, will likely need another review in a bit

@tw15egan
Copy link
Collaborator

Okay, just ping me when re-review is needed

@emyarod
Copy link
Member Author

emyarod commented Mar 19, 2019

@tw15egan carbon-design-system/carbon#2135 is now ready for review

@emyarod emyarod force-pushed the 2026-match-select-to-experimental-spec branch from 9e966e9 to 25c9e73 Compare March 19, 2019 19:20
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

For disabled, the label and helper text need to be using the color "$disabled-02"

@aagonzales
Copy link
Member

Oh ok the knob was only changing the field but I guess the labels are controlled by a different knob?

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

LGTM then!

@emyarod
Copy link
Member Author

emyarod commented Mar 19, 2019

@aagonzales can you double check again? I was still moving newly added changes from vanilla over when you reviewed, but disabling the component should also be changing label and helper text

@aagonzales
Copy link
Member

@emyarod yup! The knob is changing the label and helper text now so its good!

@laurenmrice laurenmrice merged commit 9084f78 into carbon-design-system:master Mar 19, 2019
@carbon-bot
Copy link

🎉 This PR is included in version 6.109.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@emyarod emyarod deleted the 2026-match-select-to-experimental-spec branch March 19, 2019 21:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants