-
Notifications
You must be signed in to change notification settings - Fork 322
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
feat: support multiline for EditableText #2683
feat: support multiline for EditableText #2683
Conversation
…thub.com/gabriel-amram/vibe into gabrielam/add-multiline-to-editable-text
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 the contribution! Great addition
Please see my comments :)
packages/core/src/components/EditableText/__stories__/EditableText.stories.module.scss
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableText/__stories__/EditableText.stories.module.scss
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableTypography/EditableTypography.module.scss
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableTypography/EditableTypography.module.scss
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableText/__stories__/EditableText.stories.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableText/__stories__/EditableText.stories.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/EditableText/__tests__/EditableText.test.tsx
Outdated
Show resolved
Hide resolved
…thub.com/gabriel-amram/vibe into gabrielam/add-multiline-to-editable-text
setInputHeight( | ||
Math.max( | ||
textarea.scrollHeight + textareaBorderBoxSizing.current, | ||
textareaLineHeight.current + textareaBorderBoxSizing.current |
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.
Just one thought, looks like the only difference in the dimensions is 2px, and there's no difference in paddings/line heights etc between the different variants, so looks like doing just:
setInputHeight(textarea.scrollHeight + 2);
works exactly the same and it can save the getComputeStyle and the other calculations in calculateTextareaHeightAttrs
(which already have some default static numbers). Giving it auto
in the beginning will already make it take the the currect box size. WDYT?
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.
actually the question gets back to you :)
technically its only 2px because the border-top/bottom-width is 1px (or 2px overall)
border-box should have included the border as well as the padding, but it doesn't
do we want to just rely on always having this border/padding setting and that the user cannot add anything on their own using overwriting css?
@talkor
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.
Well it is border-box, it's just that scrollHeight
is based purely on the content and padding of the element's scrollable content, without the borders.
We don't want users to override CSS as it can make design inconsistent so I wouldn't take it into consideration, generally speaking we support the variants we officially provide so it's ok :)
@gabriel-amram
Looks great @gabriel-amram ! Only one thought which I commented, and I did some quick QA and found two things worth checking:
|
…thub.com/gabriel-amram/vibe into gabrielam/add-multiline-to-editable-text
@talkor |
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!
IMO worth simplifying the height calculation nevertheless
This PR introduces the ability to support multiline in EditableText
if multiline is passed it will render a "textarea" instead of an input that cannot be resized by the user, and instead will grow/shrink according to its content up to a certain max-width or max-height defined by the container.
When using Shift+Enter a new line will be added. Hitting Enter will defocus and change the value
This is useful for users who might want to have a long description and would like to be able to edit it and change the content with line breaks.
When multiline is used, ellipsis is removed from the "view" block and from the "textarea".