-
Notifications
You must be signed in to change notification settings - Fork 26
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(localized-multiline-text-input): add support for isCondensed pro… #2793
feat(localized-multiline-text-input): add support for isCondensed pro… #2793
Conversation
🦋 Changeset detectedLatest commit: a7a114f The changes in this PR will be included in the next version bump. This PR includes changesets to release 96 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@CarlosCortizasCT There is some weirdness with this one that I can't figure out. I'm not sure if it has to do with the 3rd party library that we use for this resizable text area. But this !important style for height is getting set somewhere that I think may be calculated based on the initial rendering. This means if you toggle isCondensed in storybook, it doesn't recalculate. Not sure if this is something I need to handle? I did confirm if I have isCondensed turned on, and refresh storybook, it does render the inputs correctly. 🤷♀️ |
@Sarah4VT After investigating this case, it appears that the observed behavior results from the underlying ui-kit/packages/components/inputs/input-utils/src/multiline-input/multiline-input.tsx Line 106 in 2c9b378
which prevents re-renders. In my view, it's very unlikely for the isCondensed prop to change once this component is rendered, so I think this behavior is negligible.Another option is switching to cacheMeasurements={false} but I'm not sure if this might cause any issues.
@commercetools/shield-team-design-system what is your take on that? |
I'm wondering if we actually need the |
Thank you Kacper, I noticed it too and was playing around with it a little bit to be sure if switching it to false actually causes any issues. At least, so far, I have not noticed any issues. |
@commercetools/shield-team-design-system I went ahead and removed the |
e59adec
to
0183476
Compare
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.
Looks good now 💯
Thank you @Sarah4VT
Thank you all for looking into this. As you already spotted, the issue is because of the usage of the I can think of different options for use to consider:
Personally I would try to avoid removing that property altogether from the |
Thanks for the feedback Carlos! I agree, I was nervous to take it out, because it was obviously added for a reason and I didn't want to lose any benefits it was giving, or break some functionality we depended on. I agree that the 2nd proposed solution could work fine, but I actually thought your 3rd solution was a little more encapsulated and took mental load off of consuming components to understand when and how to set that. Right now, we know what our one use case is for needing this value turned off, and that's in Storybook if we are toggling this value. In theory if someone else wanted to toggle this value (which I agree is very unlikely), why not handle it for them since we have to handle it for ourselves anyways. I went ahead and implemented a fix based on these thoughts. Take a look, see if you like it. If not, I can undo it and add a different solution that will match your second proposal. |
308fec2
to
13593a2
Compare
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.
Some minor feedback it we would be going with this approach.
@@ -52,6 +59,20 @@ const MultilineInput = (props: TMultiLineInputProps) => { | |||
const { onHeightChange } = props; | |||
const ref = useRef<HTMLTextAreaElement | null>(null); | |||
|
|||
const [prevIsCondensed, setPrevIsCondensed] = useState(props.isCondensed); | |||
const [cacheMeasurements, setCacheMeasurements] = useState(true); |
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.
const [cacheMeasurements, setCacheMeasurements] = useState(true); | |
const [isCacheMeasurements, setIsCacheMeasurements] = useState(true); |
// cacheMeasurements is set to false. So let's set cacheMeasurements to true by default to get the performance | ||
// benefits, unless isCondensed value has changed, in which case we will set it to false for this render. | ||
if (props.isCondensed !== prevIsCondensed) { | ||
setCacheMeasurements(false); |
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.
setCacheMeasurements(false); | |
setIsCacheMeasurements(false); |
if (props.isCondensed !== prevIsCondensed) { | ||
setCacheMeasurements(false); | ||
} else { | ||
setCacheMeasurements(true); |
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.
setCacheMeasurements(true); | |
setIsCacheMeasurements(true); |
@@ -103,7 +124,7 @@ const MultilineInput = (props: TMultiLineInputProps) => { | |||
role="textbox" | |||
minRows={MIN_ROW_COUNT} | |||
maxRows={props.isOpen ? undefined : MIN_ROW_COUNT} | |||
cacheMeasurements={true} | |||
cacheMeasurements={cacheMeasurements} |
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.
cacheMeasurements={cacheMeasurements} | |
cacheMeasurements={isCacheMeasurements} |
Thanks @Sarah4VT for your feedback 👍
My argument to avoid this is that any lines of code we add to a component increases its complexity and, thus, its maintainability. |
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 work! Tested it on the preview branch and it works as expected.
13593a2
to
16d0263
Compare
@CarlosCortizasCT I made the changes to move cacheMeasurements out to a prop on the component to allow ultimate control for consumers and set it to true by default. Let me know if I need to change anything else! |
…perty on LocalizedMultilineTextInput
…ensed by reducing value padding and label line height
…llow toggling isCondensed property to affect input height immediately
…based on whether isCondensed value has been toggled
…exposed on LocalizedMultilineTextInput
16d0263
to
13dbaf4
Compare
I ended up undoing the implementation due to Carlos's feedback, so these changes were not applied as the code was removed. |
Thanks @Sarah4VT 🙇 I've just pushed a small commit in order to use the component's default props better. Also, bear in mind we're currently not respecting the expected left margin of the text input value when the component is in condensed mode. |
@@ -69,7 +69,7 @@ storiesOf('Components|Inputs', module) | |||
} | |||
defaultExpandMultilineText={defaultExpandMultilineText} | |||
isAutofocussed={boolean('isAutofocussed', false)} | |||
cacheMeasurements={boolean('cacheMeasurements', true)} | |||
cacheMeasurements={boolean('cacheMeasurements', false)} |
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.
@CarlosCortizasCT Do you typically set the default value in storybook to false even if the default value in the component is true?
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 think it's better to use false
here because, otherwise, the isCondensed
check won't work and users won't know why.
…put back to original value and only reduce top/bottom padding when isCondensed
@CarlosCortizasCT I fixed the left/right padding to go back to the original value and only reduced the top/bottom padding on the input when isCondensed is true. Good catch! |
Thanks for all the work! Looks good from my side |
Something that I wanted to check with @FilPob is the horizontal padding on the text in the input. I think we should take the opportunity to also adjust that. Filip, what do you think? |
@CarlosCortizasCT true! good catch. should be 16px for all inputs |
… 16px to match other inputs, regardless of isCondensed
@CarlosCortizasCT @FilPob I made the change to update the horizontal padding and updated the screenshot in the description. |
@Sarah4VT awesome, looks good. thanks :) |
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.
Looks good to me 👍
Thanks Sarah!
…perty on LocalizedMultilineTextInput
Summary
Adds support for isCondensed prop on LocalizedMultilineTextInput component to display a condensed layout.
Description
Figma Design
Results with isCondensed toggled on