Skip to content
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 rowsMax bug #1369

Merged
merged 8 commits into from
May 25, 2021
Merged

Fix rowsMax bug #1369

merged 8 commits into from
May 25, 2021

Conversation

pomfrida
Copy link
Contributor

Resolves #1367

pomfrida added 2 commits May 25, 2021 09:58
bug caused by function only triggered by a key event
Still flickers if you delete default text and edit again
@pomfrida pomfrida self-assigned this May 25, 2021
@pomfrida pomfrida changed the title [WIP] Fix rowsMax bug Fix rowsMax bug May 25, 2021
@pomfrida pomfrida marked this pull request as ready for review May 25, 2021 09:49
@pomfrida pomfrida requested review from mimarz, vnys and wenche as code owners May 25, 2021 09:49
Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest we remove targetEl.style.height = 'auto' from the autoReisze hook as its no longer needed?

@@ -14,13 +14,22 @@ export const useAutoResize = (
newHeight = Math.max(targetEl.scrollHeight, newHeight)
if (maxHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary as you are already checking it on line 13?

Copy link
Contributor Author

@pomfrida pomfrida May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetEl.style.height = 'auto' is needed here too, for it to autoresize correctly when user deletes text. It's to rewrite the height when it's set before.

The nested maxHeight check is also needed, as you can see its if parent is !maxHeight || maxHeight > newHeight, this logic is needed for whenever maxHeight > newHeight only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improvements fixed and tested so ready again for review @mimarz

@pomfrida pomfrida requested a review from mimarz May 25, 2021 12:32
Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@pomfrida pomfrida merged commit 4ba7db2 into develop May 25, 2021
@pomfrida pomfrida deleted the rowsmax-bug branch May 25, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textfield does not follow rowsMax rules on initial render
2 participants