MWPW-148439 - FIX - Text wraps under icon & No space between Icon and text in RTL Locale. #2343
Conversation
sivasadobe
requested review from
salonijain3,
chrischrischris,
aishwaryamathuria,
mokimo,
seanchoi-dev,
Ruchika4 and
suhjainadobe
sivasadobe
added
bug
|
libs/styles/styles.css
Outdated
| -moz-margin-end: 8px; | ||
| -webkit-margin-end:8px; |
There was a problem hiding this comment.
Are the prefixes actually needed? Based on caniuse, the browser versions requiring them go back to 2018, we'd very likely have much bigger issues on those browsers than alignment :)
There was a problem hiding this comment.
@overmyheadandbody
Make sense.
Prefix are removed.
| .icon-block .foreground .second-column .title-row { | ||
| display: flex; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
Since you're already using flex, couldn't a column-gap be used instead of the more complex margin logic?
There was a problem hiding this comment.
This code is primarily used to fix the text under the icon.
Yes, column-gap will solve this problem on pages written by ICON BLOCK. However, if RTL is enabled, it will not fix icon spacing on other pages.
The goal is to solve the no space problem on all milo pages. As a result, we changed the margin in icon.js to margin-inline, allowing the margin to be added based on direction.
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
| @@ -70,7 +70,15 @@ function decorateContent(el) { | |||
| const textContent = el.querySelectorAll('.text-content > :not(.icon-area)'); | |||
| const secondColumn = createTag('div', { class: 'second-column' }); | |||
| textContent.forEach((content) => { | |||
| secondColumn.append(content); | |||
| let nodeToInsert = content; | |||
| const firstIcon = content.querySelector('.icon:first-child'); | |||
There was a problem hiding this comment.
Just to be sure: you're targeting the icon that is the first sibling within its parent, right? If it had a text before it (or any other element), this would no longer select it and apply the logic. Is that the expectation?
There was a problem hiding this comment.
Yes, we need to select the first icon of the space and move the child of its parent tag, only after that we can do the styling. If there is no icon at first, we don't need to do anything.
Co-authored-by: Rares Munteanu <[email protected]>
spadmasa
added
verified
inline-start|endrather thanleft|right. When the RTL locale is enabled, it will resolve the space issue for all milo pages.Resolves: MWPW-148439
Test URLs:
Milo
Illustrator
Icon-Block
Browser Capabilities (margin-inline-<start|end>)