[BD-46] fix: add min-height to form.control textarea #2818
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. |
openedx-webhooks
added
the
blended
|
Thanks for the pull request, @khudym! When this pull request is ready, tag your edX technical lead. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2818 +/- ##
==========================================
- Coverage 92.89% 92.83% -0.07%
==========================================
Files 235 235
Lines 4333 4240 -93
Branches 1053 1029 -24
==========================================
- Hits 4025 3936 -89
+ Misses 304 300 -4
Partials 4 4 ☔ View full report in Codecov by Sentry. |
khudym
force-pushed
the
hudym/fix/form-control-textarea-min-height
branch
from
02cdc51
to
d1663ce
Compare
src/Form/_index.scss
Outdated
| @@ -46,6 +46,10 @@ $select-icon-padding: .5625rem !default; | |||
| flex-grow: 1; | |||
| align-items: flex-start; | |||
|
|
|||
| .form-control { | |||
| min-height: 2.75rem; | |||
There was a problem hiding this comment.
In your proposed implementation, we roughly set the value of the minimum input height with type textarea. This may not display correctly on inputs of different sizes. See what happens if an input with type textarea has different sizes (sm, md, lg):
This problem can be solved by using the $input-height variable and its variants for different sizes ($input-height-sm, $input-height-lg), the value of which will depend on the current input parameters (line height, y-axis padding and border height). Take a look please:
src/Form/_index.scss
Outdated
| .form-control { | ||
| min-height: $input-height; | ||
| } | ||
|
|
||
| .form-control-lg { | ||
| min-height: $input-height-lg; | ||
| } | ||
|
|
||
| .form-control-sm { | ||
| min-height: $input-height-sm; | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems to me that the option of specifying a minimum height only for the textarea would be better because in your implementation we have a min-height for all .form-controls that do not need it.
| .form-control { | |
| min-height: $input-height; | |
| } | |
| .form-control-lg { | |
| min-height: $input-height-lg; | |
| } | |
| .form-control-sm { | |
| min-height: $input-height-sm; | |
| } | |
| textarea { | |
| &.form-control { | |
| min-height: $input-height; | |
| } | |
| &.form-control-lg { | |
| min-height: $input-height-lg; | |
| } | |
| &.form-control-sm { | |
| min-height: $input-height-sm; | |
| } | |
| } |
There was a problem hiding this comment.
I agree, other types of inputs cannot have problems with height.
There was a problem hiding this comment.
LGTM!
Verified by going to https://paragon-openedx.netlify.app/components/form/form-control/ and resizing the text area to be shorter than the text line height
then trying to do the same here https://deploy-preview-2818--paragon-openedx.netlify.app/components/form/form-control/ and not being able to.
|
@khudym 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
🎉 This PR is included in version 21.7.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 22.0.0-alpha.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add a min textarea height
Issue#2783
Deploy Preview
Merge Checklist
exampleapp?wittjeffandadamstankiewiczas reviewers on this PR.Post-merge Checklist