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(text-input): add inline option for TextInput (#5915) #6252

Merged
merged 3 commits into from
Jul 9, 2020
Merged

feat(text-input): add inline option for TextInput (#5915) #6252

merged 3 commits into from
Jul 9, 2020

Conversation

lucasmccomb
Copy link
Contributor

Closes #5915

Changes TextInput react component to allow for inline variant
and adds required styles. Updates TextInput Storybook page
to include inline variant in Knobs tab.

Changelog

Changed

  • TextInput React component (packages/react/src/components/TextInput/TextInput.js)
  • TextInput styles (packages/components/src/components/text-input/_text-input.scss)
  • TextInput Storybook page (packages/react/src/components/TextInput/TextInput-story.js)

Testing / Reviewing

  1. fetch/checkout branch and run storybook locally
  2. open Storybook in browser and navigate to TextInput > Default page
  3. click on Knobs tab
  4. scroll to bottom of Knobs and toggle on Inline variant (inline)
  5. observe the inline variant of input
  6. toggle on Show form validation UI (invalid) to see invalid state of input

image

image

@lucasmccomb lucasmccomb requested a review from a team as a code owner June 10, 2020 22:56
@ghost ghost requested review from joshblack and tw15egan June 10, 2020 22:56
@netlify
Copy link

netlify bot commented Jun 10, 2020

Deploy preview for carbon-elements ready!

Built with commit cb943e5

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

@netlify
Copy link

netlify bot commented Jun 10, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit cb943e5

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

@tw15egan tw15egan requested review from a team and laurenmrice and removed request for a team June 11, 2020 14:33
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

just a heads up, snapshots need to be updated to pass CI

yarn test -u

@lucasmccomb
Copy link
Contributor Author

Thanks for the heads up @emyarod 🙌

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Hey @lucasmccomb , thanks for taking the time to work on this !

Instead of doing percentage based spacing, we should follow this specs spacing for these scenarios as a default:

Artboard

@lucasmccomb
Copy link
Contributor Author

Thanks for the feedback @laurenmrice! Do you know if the 128px max-width of the helper text also applies to the label?

@laurenmrice
Copy link
Member

Yes it would apply to the label too 👍🏻 @lucasmccomb

@lucasmccomb
Copy link
Contributor Author

Requested changes have been made @laurenmrice — ready for re-review.

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looking great! Just one more thing, we need to adjust the label type token and bring it down so it is aligned with the text in the input (there might not always be helper text since it is optional, so we want to always align the label). And also make sure the spacing is 2px between the label and helper text .

Here is a spec for those changes:
Artboard

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Okay alignment looks great at the default size, but needs to also align when changed to the xl or sm sizes. then we should be good
Jun-18-2020 08-46-33

@janchild
Copy link
Contributor

@laurenmrice Just some minor content things on the images...

  • Where you have the label "Text input label", the word "input" should be lower case to keep it sentence case.
  • Use "help text" instead of "helper text".
  • Where you have "Optional helper text", you don't need a period.

That's it! Thank you.

@lucasmccomb
Copy link
Contributor Author

@laurenmrice addressed the size variant alignment issues 👍

@laurenmrice
Copy link
Member

@lucasmccomb looks good to me from a design perspective! we just need to update the story content with Jans fixes above ^

Luke McComb and others added 2 commits July 8, 2020 10:19
Changes TextInput react component to allow for inline variant
and adds required styles. Updates TextInput Storybook page
to include inline variant in Knobs tab.
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.

LGTM, thanks for the contribution! 👍 ✅

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

looks awesome! thanks so much for doing this 🙌🏻

@joshblack joshblack merged commit ac08e62 into carbon-design-system:master Jul 9, 2020
@lucasmccomb
Copy link
Contributor Author

Thanks all!

@lucasmccomb lucasmccomb deleted the 5915-lem-inline-text-input branch July 13, 2020 14:27
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.

TextInput inline style
6 participants