-
Notifications
You must be signed in to change notification settings - Fork 317
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: EditableText and EditableHeading overflowing in the catalog #2555
base: master
Are you sure you want to change the base?
fix: EditableText and EditableHeading overflowing in the catalog #2555
Conversation
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 @mashazyu ! Please see my comment
packages/core/src/components/EditableTypography/EditableTypography.module.scss
Outdated
Show resolved
Hide resolved
@@ -1,11 +1,12 @@ | |||
@import "~monday-ui-style/dist/mixins"; | |||
|
|||
.editableTypography { | |||
display: inline-flex; | |||
display: block; |
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.
note:
I still changed display to block because it was requested.
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.
Seems like this causes some issues, see video attached:
Screen.Recording.2024-11-04.at.9.50.48.mp4
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.
Thank you for testing! I think changing display to 'block' caused that behavior. I reverted this change and I this issue is fixed for me locally - I verified it locally and ran lint & tests. Does proposed change fix the issue for you?
// Shifts the component to align the text with the container | ||
margin-left: -6px; | ||
// Adds 6 px removed above to the element width to avoid unnecessary overflow | ||
max-width: calc(100% + 6px); |
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.
question:
@talkor
First of all, thank you very much for the quick feedback and pointing out accessibility issues.
I didn't go with "display: block" initially because it didn't fix the overflowing issue. I looked further and realized that what's causing this issue is this negative margin-left in combination with max-width set to 100%. Adding missing 6px to max-width fixes the issue without chaning the display value.
Wdyt about this approach to fixing this issue and do you think the display value still needs to be updated? Or maybe I should also look into how element is centered?
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 @mashazyu ! Looks like a great solution. Since there's a change in the component we'll need to test it a bit to make sure it doesn't break anything. Assuming everything is ok I'll merge it. In the meantime I've added the hacktoberfest accepted tag. Thank you so much for this!
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.
Great direction! But it breaks the component
…ing editeble field, when editing
Resolves #2471
Details
Based on this tip I updated editableTypography style.
The following checks pass locally:
To verify that changes work, I checked catalog locally and it looks like overflow works correctly (see screenshots below)
Questions: