-
Notifications
You must be signed in to change notification settings - Fork 39
Add Escaped Placeholders #5448
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
base: main
Are you sure you want to change the base?
Add Escaped Placeholders #5448
Conversation
| .placeholder-unsafe { | ||
|
|
||
| @extend %placeholder; | ||
| border-radius: 0; |
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 wouldn't bother with border radius on the right-hand side, just like we don't for optional content placeholders, and only do the overrides for it as 11px gets inherited from %placeholder
border-bottom-right-radius: 0;
border-top-right-radius: 0;
| border-bottom-right-radius: 8px; | ||
| box-shadow: inset 0.47em 0 0 0 #FFF, inset 0 -0.05em 0 0 #FFF, inset 0 0.26em 0 0 #FFF; | ||
|
|
||
| .sms-message-wrapper & { |
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.
when in preview, the entire placeholder with ::unsafe is highlighted. intentional?
seems due to in not being split up like in in the the textbox, as markup is
<span class="placeholder">((message::unsafe))</span>
all the content there comes from the utils
so i think if we want consistency then placeholder markup splitting might need to be done in utils
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.
Yes, I've made those changes in utils but haven't brought them in yet. Happy to hold off deploying until they're ready to merge too
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.
cool, i guess bringing it that version of utils in this PR would make sense, so you can check that both the placeholder styling in the message wrapper (above) looks ok as well
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 can move this into draft until that's ready.
Pinned to the latest commit hash of the utils change so you can see what it's doing though
The normal placeholder css class includes a box shadow that accounts for trailing parentheses so that they do not get highlighted. This is the wrong behaviour for unsafe placeholders because it would result in the last character of a placeholders variable name not being highlighted
…aceholders when editing templates javascript should dynamically style text so that the background of a placeholders variable name is highlighted. This commit extends the regular expression to include the `::` delimeter of unsafe placeholders, let's us split the placeholder string up based on this capturing group so that the highlighted variable is styled properly
8b0ca5b to
8a68558
Compare
Best reviewed commit by commit:
adds an extra css class
placeholder-unsafefor highlighting the variable name of unsafe placeholders when editing and viewing templates, and updates the enhanced textbox javascript to dynamically style correctly when editing templates.