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

feat(textInput, textArea, Select): move classNames to outer wrapper #9502

Merged
merged 10 commits into from
Sep 2, 2021
Merged

feat(textInput, textArea, Select): move classNames to outer wrapper #9502

merged 10 commits into from
Sep 2, 2021

Conversation

andreancardona
Copy link
Contributor

@andreancardona andreancardona commented Aug 18, 2021

Closes #9465

Uses the feature-flags package to move the className prop to the outer wrapper.

Changelog

New

  • Moved the className prop to the outermost wrapper so that all elements inside are targetable via css

Changed

  • If the className prop was being added inside an element, it now returns null when the V11 feature flag is enabled. Instead, it is placed on the outer wrapper

Testing / Reviewing

Test the following components, which each have a test story: Class Name Change Test that will be removed before merging:

  • Select
  • TextArea
  • TextInput

Ensure className is placed on the outer wrapper if wrapped in the FeatureFlag element. className should remain in the same spot otherwise

@netlify
Copy link

netlify bot commented Aug 18, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 8714b90

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6130ed3f3d2bfc0007f56fc8

😎 Browse the preview: https://deploy-preview-9502--carbon-react-next.netlify.app/

@netlify
Copy link

netlify bot commented Aug 18, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 60c5512

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/611d4929a5e13b0008fcb295

😎 Browse the preview: https://deploy-preview-9502--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 18, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 8714b90

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6130ed3f4d41b1000709a0e0

😎 Browse the preview: https://deploy-preview-9502--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 18, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 8714b90

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6130ed3fca168d00070cfbbe

😎 Browse the preview: https://deploy-preview-9502--carbon-components-react.netlify.app

@tw15egan
Copy link
Member

tw15egan commented Aug 18, 2021

Looks like TextInput is still adding the class to the inner input. I think we need to conditional render it [here] (https://github.com/carbon-design-system/carbon/pull/9502/files#diff-7820afc8fe28f023d88aad65b058606d670d1451929594acfce5505010358fb2R56) (L56)

const textInputClasses = classNames(`${prefix}--text-input`, [enabled ? null : className]

Screen Shot 2021-08-18 at 9 43 55 PM

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.

LGTM once test stories are removed

@andreancardona andreancardona merged commit 6b6933c into carbon-design-system:main Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update components to place className on outer node
3 participants