-
-
Notifications
You must be signed in to change notification settings - Fork 729
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 expand notes field when editing order #13214
base: master
Are you sure you want to change the base?
Fix expand notes field when editing order #13214
Conversation
- notes field was a children of label element - due to this its size was bound by the label element - now this commit makes it a sibling, making it occupy the width of column width
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.
Nice one 👍
%label | ||
= t(".note_label") | ||
= text_field_tag :note, @order.note, { maxLength: 280, data: { "input-char-count-target": "input" } } | ||
= text_field_tag :note, @order.note, { maxLength: 280, data: { "input-char-count-target": "input" } } |
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.
Now we have more space for the input but the label is not associated to the input anymore. You can use Rails helpers to assign the id, I guess.
I'm also wondering if we should make the text area span more rows. Three rows should be enough to the allowed 280 characters. Four rows max for 70 chars per row but I think that we can assume that most people won't reach the limit. Two rows would also be enough, maybe with a control to adjust the size?
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.
@mkllnk - Sure thing on the id part, will do that.
On the text area suggestion, I think let's confirm this from @tschumilas and @RachL .
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 it was possible to leave the current 280 character limit that would be idea. I regularly use over 250 characters.
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 @tschumilas for the feedback.
We are now using a textarea field which supports the spaces and new lines as well. Here's the demo:
Screen.Recording.2025-03-24.004438.mp4
@mkllnk - I've addressed your comments, it's ready for review. Thanks.
Here: 806965b
…matting in the notes
@@ -15,7 +15,8 @@ | |||
- if order.note.present? | |||
%strong | |||
= t(".note_label") | |||
= order.note | |||
%pre{ style: "font-family: inherit;" } |
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's needed because now we have a text area field and sure is now able to enter new lines in the notes.
We have to use this pre
tag to support the display of new lines. Moreover, this tag uses a different font-family by default, so using inherit to use the default application one.
What? Why?
This PR resolves an issue where the label element constrains the notes field, limiting its width.
Previously, the notes field was a child of the label element, restricting its size. This PR updates the structure so that the notes field is now a sibling of the label, allowing it to occupy the full column width.
Demo:
Screen.Recording.2025-03-24.004438.mp4
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):