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

[core] feat(TagInput): resizable input that scales as you type #5966

Merged
merged 10 commits into from
Feb 27, 2023
Merged

[core] feat(TagInput): resizable input that scales as you type #5966

merged 10 commits into from
Feb 27, 2023

Conversation

JosephChotard
Copy link
Contributor

Checklist

  • [N/A] Includes tests
  • [N/A] Update documentation

Changes proposed in this pull request:

Currently the TagInput component (used by MultiSelect2) does not expand to accomodate the text the user is typing. This is because input elements don't scale with their input. This causes annoying situations where the user cannot see what they are typing when there is space to display it.
To address this issue, I created a small ResizeableInput component (could be included in TagInput but this makes the intent clearer). This new component acts like a regular input element for every other component but it has some coding magic that causes it to fit the size of it's content. It will never grow bigger than it's closest parent with a position: relative.

Reviewers should focus on:

This applies to both TagInput and MultiSelect
I checked that:

  • The input never stretches wider than the parent
  • The input scales with it's content
  • The input is never smaller than 80px
  • The existing props still work (large, disabled, add on blur, ...)

This PR does not introduce changes to the API and is fully backwards compatible.

Screenshot

Old Input scaling:
numbers - old
New Input scaling:
numbers - new

Old input text being cut off
wrap old

New input text wrapping
wrap new

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Nice, this seems to work really well in the docs preview build!

Looks like the tests are failing because we're using shallow rendering in many of the TagInput tests, and the <ResizeableInput> children don't get rendered. We can fix that by switching to mount() instead of shallow().

@adidahiya adidahiya changed the title Fix tag input not scaling with the input [core] feat(TagInput): use resizable input that scales as you type Feb 22, 2023
@JosephChotard
Copy link
Contributor Author

enzymejs/enzyme#1473
enzymejs/enzyme#1081
Changed the tests to use mount and modified a few .simulate calls as they caused the tests to hang (not sure why), instead I call the prop directly, I don't believe we need to test the event passing as that's React's responsibility.

@JosephChotard
Copy link
Contributor Author

@adidahiya done all the changes, lmk if there's anything else

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

looking good, just a few more minor things

packages/core/src/common/classes.ts Outdated Show resolved Hide resolved
packages/core/src/components/_index.scss Outdated Show resolved Hide resolved
packages/core/src/components/tag-input/resizableInput.tsx Outdated Show resolved Hide resolved
@JosephChotard
Copy link
Contributor Author

@adidahiya thanks for all the feedback, should be addressed now

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the thorough PR @JosephChotard 👍

@adidahiya adidahiya changed the title [core] feat(TagInput): use resizable input that scales as you type [core] feat(TagInput): resizable input that scales as you type Feb 27, 2023
@adidahiya adidahiya merged commit c008b85 into palantir:develop Feb 27, 2023
@JosephChotard JosephChotard deleted the jc/input-tag-scaling branch February 28, 2023 06:29
@nerdstep
Copy link
Contributor

Looks like there might be a regression with the input width not expanding to the full width of the component when using the fill prop:

image


return (
<span>
<span id="hide" ref={span} className={Classes.RESIZABLE_INPUT_SPAN}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this id prop is needed / where it's used, but it's causing aXe a11y failures because all ids must be unique.

Must either remove id prop, or use uniqueId for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR: #5984

@adidahiya
Copy link
Contributor

@nerdstep thanks for pointing out the regression. Fix here: #5985

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.

None yet

4 participants