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

TextInput v11 Styles #1889

Conversation

SimpleProgrammingAU
Copy link
Contributor

Tasks completed and ready for review

Tasks outstanding

I also alphabetised the prop list for legibility. I figured this was a good point to submit a PR since all v11 style stuff is done, just need to tidy up the loose ends with the other bugs.

@brunnerh
Copy link
Contributor

I would recommend doing things that cause a lot of non-functional differences, like sorting properties, in separate PRs. That makes it easier to review as there is less noise.

@theetrain
Copy link
Collaborator

@brunnerh that's a good tip; I moved this outcome to its own ticket: #1891.

I'll try reviewing this PR as-is for now.

@SimpleProgrammingAU
Copy link
Contributor Author

I would recommend doing things that cause a lot of non-functional differences, like sorting properties, in separate PRs. That makes it easier to review as there is less noise.

I thought the same thing after the fact. I sorted the properties because I was having so much difficulty finding them in the code as I was reviewing against the React component.

Copy link
Collaborator

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

Thanks for helping standardize some props and events while adopting v11 styles.

Note to self:

TextInput Breaking changes

  • on:mouseover, on:mouseenter, and on:mouseleave events are no longer forwarded. Use on:pointerup, on:pointerover, on:pointenter, and on:pointerleave instead
  • size no longer supports xl value
  • on:input and on:change are forwarded as native events
  • values in <TextInput type="number" /> are no longer cast as numbers; they use the default native string type

TextInput new features

  • Carbon v11 styles
  • New counter and maxCount props to display a character count
  • New slots: helperText, invalidText, warnText
  • light prop is deprecated. Use the forthcoming Layer componenet instead
  • on:input and on:change are forwarded as native events
  • New 'pass-through' props: labelAttributes and inputAttributes.

TextInputSkeleton Breaking changes

  • on:mouseover, on:mouseenter, and on:mouseleave events are no longer forwarded. Use on:pointerup, on:pointerover, on:pointenter, and on:pointerleave instead

TextInputSkeleton new features

  • New 'pass-through' prop: divAttributes

package.json Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
@@ -94,6 +124,14 @@
dispatch("change", parse(e.target.value));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these onChange and parse methods should be removed. We cast
number-type inputs, but the React library doesn't. Maybe this can be handled
in userland, especially since <input type="number" /> when submitted in a
<form> is read server-side as a string.

Demo: https://www.sveltelab.dev/csng8xsyb7bhy56

cc @metonym

src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
src/TextInput/TextInput.svelte Outdated Show resolved Hide resolved
@theetrain
Copy link
Collaborator

Sorry I submitted my review prematurely. I was using VSCode to review and didn't realize 'request changes' would immediately submit. I'll follow up with the remainder of my review.

image

Copy link
Collaborator

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

@SimpleProgrammingAU review done; apologies for the repeat reviews - VSCode's review UI is kinda unintuitive.

Last thing to note:

Over to you!

docs/src/pages/components/TextInput.svx Show resolved Hide resolved
SimpleProgrammingAU and others added 5 commits January 16, 2024 18:53
Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <[email protected]>
Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <[email protected]>
Explicitly define default value for `size`

Co-authored-by: Enrico Sacchetti <[email protected]>
@SimpleProgrammingAU
Copy link
Contributor Author

Thanks for the feedback. Back to you :)

Copy link
Collaborator

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

Great work 👍

I updated change notes: #1889 (review)

Changes work well in all themes, and within FluidForm.

I'll leave this open for a few hours in case others want to have a look, then will release.

@theetrain theetrain merged commit c436dbd into carbon-design-system:next/v11-styles Jan 17, 2024
1 check passed
Comment on lines -3 to -4
* @event {null | number | string} change
* @event {null | number | string} input
Copy link
Contributor

Choose a reason for hiding this comment

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

Such changes will certainly break existing codebases.

I think we should adopt v11 styles primarily while supporting existing codebases as much as possible.

Once v11 styles are merged to master we can then work towards feature parity with the canonical implementation (React).

V11 styles will be even more delayed if we try to achieve multiple goals at the same time.

Copy link
Contributor Author

@SimpleProgrammingAU SimpleProgrammingAU Jan 17, 2024

Choose a reason for hiding this comment

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

This is something that will happen eventually anyway. I'd make the case that given we're already making breaking changes, it may as well all come at once.

Most of the fixes to codebases will be relatively trivial (i.e. largely find & replace and copy-paste).

I would add that file-by-file, the changes haven't been onerous from a coding perspective, either.

Copy link
Contributor

@gregorw gregorw Jan 18, 2024

Choose a reason for hiding this comment

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

I’m glad you’re pushing this project forward. We want the same thing. My worry is that once we open a PR from next/v11-styles → master the reviewer, i.e. @metonym, will have a very hard time assessing all these to some extend unnecessary changes. What I’m saying is that we will have v11 styles on the master branch earlier if we kept the change to a minimum.

This is something that will happen eventually anyway.

We could propose this very change to master then. It’s unrelated to v11 styles. Feature parity with the canonical implementation is a separate issue.

I'd make the case that given we're already making breaking changes, it may as well all come at once.

  1. I don’t think we should make breaking changes. At least I tried not to make any unnecessary breaking changes.
  2. I don’t think making them all at once is a good idea because it makes reviewing, writing release notes and upgrading apps very hard.

Most of the fixes to codebases will be relatively trivial (i.e. largely find & replace and copy-paste).

Are you running a carbon-components-svelte app in production? We do. If we suddenly miss events on TextInput we cannot just run find & replace. It’ll break the application.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate the discussion. I hope I have addressed Gregor's concerns in #1872 (comment) - let's continue there.

@gregorw gregorw mentioned this pull request Jan 18, 2024
4 tasks
metonym pushed a commit that referenced this pull request Mar 23, 2024
* Initial commit

* Fixes [FluidForm] TextInput error icon is misplaced #1667

* Contributes to [TextInput] helperText enhancements #1633

* Adopts Standardize props and events #1621

* Added slots for Standardize props and events #1621

* Added pointer events, updated skeleton TextInput v11 #1888

* Address a bug in the word counter regex

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <[email protected]>

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <[email protected]>

* Update src/TextInput/TextInput.svelte

Explicitly define default value for `size`

Co-authored-by: Enrico Sacchetti <[email protected]>

* Adopted suggested changes

* Updated `TextInput.test`; added forgotten files from previous

---------

Co-authored-by: Samuel Janda <[email protected]>
Co-authored-by: Enrico Sacchetti <[email protected]>
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.

4 participants