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

feat(helper-text): move helper text below form inputs #5867

Merged
merged 11 commits into from
May 14, 2020
Merged

feat(helper-text): move helper text below form inputs #5867

merged 11 commits into from
May 14, 2020

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Apr 15, 2020 •
edited
Loading

Closes #5279

This PR moves the helper text for form inputs below the input themselves.

Changelog

Changed

  • Helper text now uses $text-02
  • Moved helper text below form inputs
  • Changed margin on helper text
  • Updated vanilla examples

Removed

  • Wrong line-height on form requirement text
  • Extra styles in _textarea.scss

Testing / Reviewing

Check form elements and make sure they render correctly when invalid and in all other states.

  • Combobox
  • Dropdown
  • Form
  • Listbox
  • Multiselect
  • Select
  • Text Input
  • Textarea

@tw15egan
Copy link
Collaborator Author

@dakahn is there anything I need to add to the helper text for a11y purposes?

@netlify
Copy link

netlify bot commented Apr 15, 2020

Deploy preview for carbon-elements ready!

Built with commit cecc972

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

@netlify
Copy link

netlify bot commented Apr 15, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 84b7a11

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

@netlify
Copy link

netlify bot commented Apr 15, 2020

Deploy preview for carbon-components-react ready!

Built with commit cecc972

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

@netlify
Copy link

netlify bot commented Apr 15, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 84b7a11

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

@tw15egan tw15egan marked this pull request as ready for review April 16, 2020 18:37
@tw15egan tw15egan requested a review from a team as a code owner April 16, 2020 18:37
@ghost ghost requested review from aledavila and asudoh April 16, 2020 18:37
@tw15egan tw15egan requested a review from jeanservaas April 16, 2020 18:37
jeanservaas
jeanservaas approved these changes Apr 17, 2020
View reviewed changes
Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

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

lgtm

dakahn
dakahn approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

I think since we're just moving stuff around here we should be good. Typically you'd want aria-describedby attributes on the inputs with IDs referencing the helper text. If we're missing those we can patch them up in another PR though.

@dakahn
Copy link
Contributor

dakahn commented Apr 20, 2020

Anyway we can sit on this one for one more day while we get the Downshift upgrade in? The merge conflicts wont be a huge deal if we need this ASAP for something else though.

@tw15egan
Copy link
Collaborator Author

@dakahn for sure will wait to merge this until yours goes through. It will be much easier for me to fix merge conflicts 👊

@joshblack joshblack mentioned this pull request Apr 21, 2020
Closed
@janhassel janhassel mentioned this pull request Apr 24, 2020
Merged
@dakahn
Copy link
Contributor

dakahn commented May 8, 2020

🙏 sorry for the conflicts. Happy to help out if any of the changes seem weird though.

@tw15egan tw15egan force-pushed the helper-text-swap branch from 1514cd3 to 4346f81 Compare May 12, 2020 17:30
@tw15egan
Copy link
Collaborator Author

@dakahn @aledavila @jeanservaas updated this PR to account for the large Downshift changes we recently made, just want y'all to give it another look and make sure it still looks good! 👍

jeanservaas
jeanservaas reviewed May 12, 2020
View reviewed changes
Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

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

@tw15egan

Looks good, I did notice one kind of weird thing... it's only for Text Area... and it's only in Safari, but the spacing looks off

all other components look fine... I also inspected Text Area in Chrome and it was fine... weird

image

but both the helper text and the input label look a little further away from the field.

@tw15egan tw15egan force-pushed the helper-text-swap branch from 67fe5ff to a2ddc09 Compare May 13, 2020 17:44
@tw15egan
Copy link
Collaborator Author

@jeanservaas nice catch, it seems like it was a safari only bug. I threw a height: 100%; on the textarea and it seems to render correctly now 👍

dakahn
dakahn approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Thanks so so so so so so much for scrapping through those probably gnarly merge conflicts. 🙏

asudoh
asudoh reviewed May 13, 2020
View reviewed changes
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, one question; Does the older version of markup in .hbs work with the newer CSS? Thanks @tw15egan!

@tw15egan
Copy link
Collaborator Author

@asudoh older markup will still work, the margin just may be a bit tight

@joshblack joshblack merged commit e560f4d into carbon-design-system:master May 14, 2020
This was referenced Jun 29, 2020
Closed
Closed
@tw15egan tw15egan deleted the helper-text-swap branch April 28, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanservaas jeanservaas jeanservaas approved these changes

@asudoh asudoh asudoh left review comments

@dakahn dakahn dakahn approved these changes

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

enhancement[helper text]: move helper text below input fields and dropdowns
6 participants
@tw15egan @dakahn @asudoh @aledavila @jeanservaas @joshblack