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(textfield): add rows attribute #3356

Merged
merged 9 commits into from Jun 25, 2023
Merged

fix(textfield): add rows attribute #3356

merged 9 commits into from Jun 25, 2023

Conversation

ingorichter
Copy link
Contributor

@ingorichter ingorichter commented Jun 22, 2023

Description

Adding the rows attribute to enable a multiline Textarea to show up with a predefined amount of rows. We need this feature for an existing feature where we replace all react-spectrum components with spectrum-web-components.

Related issue(s)

Motivation and context

Product required for Adobe Share Dialog

How has this been tested?

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@github-actions
Copy link

github-actions bot commented Jun 22, 2023

Tachometer results

Chrome

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 616 kB 209.00ms - 211.92ms - unsure 🔍
-1% - +1%
-1.71ms - +2.05ms
branch 617 kB 209.11ms - 211.47ms unsure 🔍
-1% - +1%
-2.05ms - +1.71ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 461 kB 114.21ms - 115.83ms - unsure 🔍
-2% - +1%
-1.89ms - +0.84ms
branch 462 kB 114.45ms - 116.64ms unsure 🔍
-1% - +2%
-0.84ms - +1.89ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 446 kB 159.05ms - 161.87ms - unsure 🔍
-1% - +1%
-2.33ms - +1.74ms
branch 447 kB 159.29ms - 162.22ms unsure 🔍
-1% - +1%
-1.74ms - +2.33ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 410 kB 68.14ms - 69.58ms - unsure 🔍
-2% - +1%
-1.05ms - +0.90ms
branch 411 kB 68.28ms - 69.59ms unsure 🔍
-1% - +2%
-0.90ms - +1.05ms
-
Firefox

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 616 kB 523.28ms - 554.92ms - unsure 🔍
-4% - +4%
-23.13ms - +22.77ms
branch 617 kB 522.65ms - 555.91ms unsure 🔍
-4% - +4%
-22.77ms - +23.13ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 461 kB 254.53ms - 288.59ms - unsure 🔍
-6% - +10%
-15.28ms - +25.88ms
branch 462 kB 254.70ms - 277.82ms unsure 🔍
-9% - +6%
-25.88ms - +15.28ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 446 kB 416.44ms - 437.92ms - unsure 🔍
-7% - +2%
-29.49ms - +7.37ms
branch 447 kB 423.27ms - 453.21ms unsure 🔍
-2% - +7%
-7.37ms - +29.49ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 410 kB 159.67ms - 185.21ms - unsure 🔍
-12% - +9%
-20.72ms - +15.96ms
branch 411 kB 161.65ms - 187.99ms unsure 🔍
-9% - +12%
-15.96ms - +20.72ms
-

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

This looks good to me. We've got one piece left and we'll be ready to merge. Can you checkout the docs on Visual Regression Testing and update the golden images hash? Feel free to message me here or in Slack if any of that is unclear.

Thanks!

Comment on lines 162 to 163
block-size: auto;
resize: none;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines should already be covered by line 157/8 from above. Can we remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I cleaned it up. Thanks for spotting it

@@ -97,6 +97,9 @@ export class TextfieldBase extends ManageHelpText(Focusable) {
@property({ type: Boolean, reflect: true })
public readonly = false;

@property({ type: Number })
public rows = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense to normalize across the file, thanks for calling that out!

@Westbrook Westbrook changed the title feat(textfield): add rows attribute fix(textfield): add rows attribute Jun 25, 2023
Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Thanks @ingorichter this looks good to me. I can't guarantee a pre-shutdown release, but if not, look for this to be available shortly thereafter. Cheers!

@Westbrook Westbrook merged commit 1ee1c37 into main Jun 25, 2023
47 checks passed
@Westbrook Westbrook deleted the ingo/add-rows-property branch June 25, 2023 16:52
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.

[Feat]: Add rows attribute to enable multiline Textarea with a predefined amount of visible lines
2 participants