-
Notifications
You must be signed in to change notification settings - Fork 72
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
fix: associate labels with autosuggest input #2755
fix: associate labels with autosuggest input #2755
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2755 +/- ##
==========================================
+ Coverage 92.82% 92.84% +0.01%
==========================================
Files 235 235
Lines 4237 4247 +10
Branches 1029 1032 +3
==========================================
+ Hits 3933 3943 +10
Misses 300 300
Partials 4 4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, the icon button (dropdown arrow) is behaving as expected. As well as having the label associated with the input box correctly. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for missing this earlier! +1 to everything Cindy said -- I saw the same things when testing locally so this LGTM.
Out of curiosity, would we want to get rid of mentions/uses of floatingLabel
within the Form.Autosuggest component at this point since labels are now associated with the input in another way?
Good call! I don't think removing the |
Sounds good -- thanks, Brian! |
f9ee3f7
to
ea7a1a6
Compare
src/Form/form-autosuggest.mdx
Outdated
Option with custom onClick | ||
</Form.AutosuggestOption> | ||
</Form.Autosuggest> | ||
<Form.Group> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this mdx
file should have 2 indent spaces (currently 4). This can be seen by looking at other mdx
files of the Form component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I updated all the jsx in from-autosuggest.mdx
https://github.com/openedx/paragon/compare/ea7a1a6fde65eac7566541d5d8be3a5b7af526dd..a1f5abbcae9703bdb7bd1e80f5f093a84d7aa36b
ea7a1a6
to
a1f5abb
Compare
🎉 This PR is included in version 21.11.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 22.0.0-alpha.24 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Fixes #2436
Merge Checklist
wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist