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(Input): Added htmlSize prop to set native size attribute #1419

Merged
merged 7 commits into from Jul 25, 2023

Conversation

adamhock
Copy link
Contributor

@adamhock adamhock commented Jul 19, 2023

Changes

Added htmlSize prop to the Input component which handles the native size attribute in <input>.

Closes #973

Testing

Added new unit test

Docs

Updated documentation to explain new prop
Changeset added

@adamhock adamhock requested review from a team as code owners July 19, 2023 20:05
@adamhock adamhock requested review from gretanausedaite and LostABike and removed request for a team July 19, 2023 20:05
@adamhock adamhock self-assigned this Jul 19, 2023
@gretanausedaite gretanausedaite added this to the React 3.0 milestone Jul 20, 2023
@mayank99
Copy link
Contributor

maybe we can leave size as-is and then add htmlSize for native attribute? we can explain the difference in the jsdoc and our documentation site.

reason: it is way more common to use the size prop to change height, than it is to use the native size attribute to constrain width.

@adamhock adamhock changed the title fix(Input): Renamed size prop to not conflict with native size attribute fix(Input): Added htmlSize prop to set native size attribute Jul 20, 2023
@adamhock
Copy link
Contributor Author

maybe we can leave size as-is and then add htmlSize for native attribute? we can explain the difference in the jsdoc and our documentation site.

reason: it is way more common to use the size prop to change height, than it is to use the native size attribute to constrain width.

Great idea! This has been updated.

Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

Everything looks fine, I just have a question. The native size attribute of <input> is to determine how many characters will be visible right? However, because we have inline-size = 100% within our <Input/> component styling, setting this size (or htmlSize) will not do anything unless user specifically override our component inline-size styling. Is this intended?

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

LGTM.

.changeset/shaggy-hornets-enjoy.md Outdated Show resolved Hide resolved
packages/itwinui-react/src/core/Input/Input.tsx Outdated Show resolved Hide resolved
@mayank99
Copy link
Contributor

The native size attribute of <input> is to determine how many characters will be visible right? However, because we have inline-size = 100% within our <Input/> component styling, setting this size (or htmlSize) will not do anything unless user specifically override our component inline-size styling. Is this intended?

It is intended that our input fills up the width of the container, because otherwise it would have some weird default width. So yeah, to make use of this size (htmlSize) attribute, the developer would need to use width: unset or inline-size: unset. We could mention this in the jsdoc comment for our prop.

@adamhock
Copy link
Contributor Author

The native size attribute of <input> is to determine how many characters will be visible right? However, because we have inline-size = 100% within our <Input/> component styling, setting this size (or htmlSize) will not do anything unless user specifically override our component inline-size styling. Is this intended?

It is intended that our input fills up the width of the container, because otherwise it would have some weird default width. So yeah, to make use of this size (htmlSize) attribute, the developer would need to use width: unset or inline-size: unset. We could mention this in the jsdoc comment for our prop.

Could we apply width: unset or inline-size: unset stylings to <Input> if an htmlSize prop is passed?

@mayank99
Copy link
Contributor

Could we apply width: unset or inline-size: unset stylings to <Input> if an htmlSize prop is passed?

Overengineering it imo. I do not expect this attribute to be used commonly enough to justify the extra code. If somebody finds the current behavior confusing, we can always revisit in the future.

Oh and slight typo in 6ef5d74 btw. Should say "properties" instead of "attributes".

Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

LGTM :D

@adamhock adamhock added this pull request to the merge queue Jul 25, 2023
Merged via the queue into dev with commit bd83460 Jul 25, 2023
16 of 17 checks passed
@adamhock adamhock deleted the adam/native-input-size branch July 25, 2023 03:08
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

v3: Input size prop overrides native size attribute
4 participants