-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fit-textareas
- Apply to issue template placeholders
#7530
Conversation
source/features/fit-textareas.tsx
Outdated
if (textarea.classList.contains('js-size-to-fit')) { | ||
textarea.classList.replace('js-size-to-fit', 'rgh-fit-textareas'); | ||
} else { | ||
textarea.classList.add('rgh-fit-textareas'); | ||
} |
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.
also happy to do this differently, if desired:
- only 1 line of
textarea.classList.add('rgh-fit-textareas');
(instead ofif/else
) - add CSS to
.rgh-fit-textareas
class insource/features/fit-textareas.css
to override the.js-size-to-fit
styles
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.
No conditions are necessary, you can just remove the class (whether it exists or not) and add the other. No "replace" call
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.
sounds good:
source/features/fit-textareas.css
Outdated
@@ -1,4 +1,5 @@ | |||
.rgh-fit-textareas { | |||
height: auto !important; |
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.
I believe this will break the manual resizing (you can drag the bottom right corner to resize it)
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.
indeed it does 🤔 😟
the reason I added it was that:
- the
height: 100px !important
below height: <pixel value>
was appearing as an inline style on some of thesetextarea
s - not sure where that was coming from though, so that may not be an unwanted thing
.issue-form-textarea {
height: 100px !important;
min-height: 100px !important;
}
but maybe I should just remove that .issue-form-textarea
class instead (and probably others, eg. PRs, Discussions, etc, hmm...
if you have any suggestions, I can also see whether those would work
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.
in addition to the styles above, there's also another block for the unfocused fields:
.form-group textarea.form-control {
width: 100%;
height: 200px;
min-height: 200px;
}
where the height: 200px
is also causing constraints there
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.
Perhaps:
- remove the first class
- make the selector stronger to override the second rule
This way you don't need to use !important
But actually manual resizing isn't important on those fields, so we don't need to do anything there. However the current code also applies to other fields.
The solution is to move height: auto !important
to a more specific selector, like .rgh-fit-textarea.issue-form-textarea
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.
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.
if the height: auto !important
with the .rgh-fit-textarea.issue-form-textarea
selector is preferable after all, I can switch to that instead
source/features/fit-textareas.tsx
Outdated
if (textarea.classList.contains('js-size-to-fit')) { | ||
textarea.classList.replace('js-size-to-fit', 'rgh-fit-textareas'); | ||
} else { | ||
textarea.classList.add('rgh-fit-textareas'); | ||
} |
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.
No conditions are necessary, you can just remove the class (whether it exists or not) and add the other. No "replace" call
source/features/fit-textareas.css
Outdated
@@ -2,3 +2,7 @@ | |||
max-height: none !important; | |||
field-sizing: content; | |||
} | |||
|
|||
textarea.rgh-fit-textareas.form-control { |
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.
Don't all textareas have this class?
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.
the focused textarea
s have this class (no .form-control
class):
js-comment-field js-paste-markdown js-task-list-field js-quick-submit FormControl-textarea CommentBox-input rgh-fit-textareas js-saved-reply-shortcut-comment-field input-monospace rgh-seen--7661699013
even if height: auto
(without !important
) was applied in future to the focused textarea
elements, it would still not cause a problem for the manual resizing, which sets an inline height
, which is higher specificity
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.
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.
ok sounds good. interesting, wonder why that didn't show up for me - maybe I didn't test correctly
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.
wonder why that didn't show up for me
My guess is that the browser isn't consistent in the injection order of the styles.
fit-textareas
: Apply to unfocused textareasfit-textareas
- Apply to issue template placeholders
Thanks for the review and merge and publish in I tested the new version just now in Chrome, and it is no longer obscuring the text in the unfocused fields 🎉 Kapture.2024-07-14.at.11.17.00.mp4 |
Closes #7522
Test URLs
Screenshot