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

feat(textfield): added name attribute to textfield #3752

Merged
merged 10 commits into from
Nov 14, 2023

Conversation

blunteshwar
Copy link
Collaborator

@blunteshwar blunteshwar commented Oct 31, 2023

Description

Added name attribute to sp-textfiled, sp-picker, sp-swicth, sp-checkbox

Related issue(s)

[Feat]: Name attribute for input components #2406

Motivation and context

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)

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.

@blunteshwar blunteshwar linked an issue Oct 31, 2023 that may be closed by this pull request
1 task
@github-actions
Copy link

github-actions bot commented Oct 31, 2023

Tachometer results

Chrome

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 511 kB 66.66ms - 67.50ms - unsure 🔍
-2% - +1%
-1.17ms - +0.46ms
branch 498 kB 66.74ms - 68.13ms unsure 🔍
-1% - +2%
-0.46ms - +1.17ms
-

checkbox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 57.63ms - 60.43ms - unsure 🔍
-4% - +3%
-2.18ms - +1.81ms
branch 412 kB 57.79ms - 60.64ms unsure 🔍
-3% - +4%
-1.81ms - +2.18ms
-

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 521 kB 103.00ms - 105.35ms - unsure 🔍
-2% - +1%
-1.84ms - +1.49ms
branch 504 kB 103.18ms - 105.53ms unsure 🔍
-1% - +2%
-1.49ms - +1.84ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 474 kB 64.26ms - 65.20ms - unsure 🔍
-0% - +1%
-0.30ms - +0.84ms
branch 460 kB 64.13ms - 64.79ms unsure 🔍
-1% - +0%
-0.84ms - +0.30ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 474 kB 119.48ms - 123.13ms - unsure 🔍
-2% - +2%
-2.30ms - +2.07ms
branch 457 kB 120.22ms - 122.61ms unsure 🔍
-2% - +2%
-2.07ms - +2.30ms
-

switch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 387 kB 27.56ms - 27.93ms - unsure 🔍
-1% - +0%
-0.38ms - +0.10ms
branch 387 kB 27.74ms - 28.04ms unsure 🔍
-0% - +1%
-0.10ms - +0.38ms
-

table permalink

Version Bytes Avg Time vs remote vs branch
npm latest 519 kB 375.54ms - 379.75ms - unsure 🔍
-0% - +1%
-1.29ms - +4.11ms
branch 506 kB 374.55ms - 377.92ms unsure 🔍
-1% - +0%
-4.11ms - +1.29ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 432 kB 41.57ms - 42.11ms - unsure 🔍
-3% - +0%
-1.24ms - +0.11ms
branch 418 kB 41.79ms - 43.02ms unsure 🔍
-0% - +3%
-0.11ms - +1.24ms
-
Firefox

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 511 kB 134.00ms - 140.08ms - unsure 🔍
-2% - +4%
-3.08ms - +5.48ms
branch 498 kB 132.83ms - 138.85ms unsure 🔍
-4% - +2%
-5.48ms - +3.08ms
-

checkbox permalink

Version Bytes Avg Time vs remote vs branch
npm latest 425 kB 121.99ms - 130.09ms - unsure 🔍
-5% - +4%
-5.97ms - +4.73ms
branch 412 kB 123.17ms - 130.15ms unsure 🔍
-4% - +5%
-4.73ms - +5.97ms
-

number-field permalink

Version Bytes Avg Time vs remote vs branch
npm latest 521 kB 236.50ms - 245.90ms - unsure 🔍
-2% - +3%
-5.71ms - +8.11ms
branch 504 kB 234.94ms - 245.06ms unsure 🔍
-3% - +2%
-8.11ms - +5.71ms
-

search permalink

Version Bytes Avg Time vs remote vs branch
npm latest 474 kB 129.94ms - 135.78ms - unsure 🔍
-4% - +2%
-6.06ms - +2.46ms
branch 460 kB 131.56ms - 137.76ms unsure 🔍
-2% - +5%
-2.46ms - +6.06ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 474 kB 228.30ms - 238.62ms - unsure 🔍
-2% - +3%
-4.21ms - +8.05ms
branch 457 kB 228.23ms - 234.85ms unsure 🔍
-3% - +2%
-8.05ms - +4.21ms
-

switch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 387 kB 66.84ms - 71.04ms - unsure 🔍
-9% - +0%
-6.67ms - +0.35ms
branch 387 kB 69.29ms - 74.91ms unsure 🔍
-1% - +10%
-0.35ms - +6.67ms
-

table permalink

Version Bytes Avg Time vs remote vs branch
npm latest 519 kB 655.40ms - 660.00ms - unsure 🔍
-3% - +0%
-17.84ms - +2.64ms
branch 506 kB 655.33ms - 675.27ms unsure 🔍
-0% - +3%
-2.64ms - +17.84ms
-

textfield permalink

Version Bytes Avg Time vs remote vs branch
npm latest 432 kB 81.47ms - 88.01ms - unsure 🔍
-6% - +5%
-4.79ms - +3.91ms
branch 418 kB 82.32ms - 88.04ms unsure 🔍
-5% - +6%
-3.91ms - +4.79ms
-

@blunteshwar blunteshwar marked this pull request as ready for review November 9, 2023 13:28
…github.com:adobe/spectrum-web-components into blunteshwar/add-name-attribute-to-input-components
Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

I have some questions. Thank you for adding tests!

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

@property({ type: String, reflect: true })
public name: string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I have a question for you. I'm trying to understand the rationale behind the decision of writing the name property type like this. If the goal is that if a user doesn't include the name attribute, then we want to explicitly state that name = undefined, right?

@property({type: String, reflect: true})
public name: string = ''; 

^ So something like this would defeat the purpose of the goal of form elements using autocomplete. Am I correct in understanding that??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Autocomplete will work just fine with both public name: string | undefined; and public name: string = '';
To answer your question, I used public name: string | undefined; because it makes more sense for name attribute to be either undefined or a valid string. When I write public name: string = ''; instead of public name: string | undefined; the name attribute can be seen in the inspect element section although the user has not given it any value.
Screenshot 2023-11-10 at 9 19 07 AM
Screenshot 2023-11-10 at 9 20 12 AM

Also when I use public name: string = ''; I get the error shown above in image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. I think that's fair. However, would

@property({type: String, reflect: true})
public name?: string; 

accomplish the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, In this case when nothing is assigned to name, instead of being undefined it would be equal to "" (empty string).

@@ -54,6 +58,7 @@ export class CheckboxBase extends Focusable {
protected override render(): TemplateResult {
return html`
<input
name=${ifDefined(this.name || undefined)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Continuing on here, could we do something like

name=${this.name ? this.name : undefined} 

instead? I think wrapping undefined in an ifDefined() doesn't make sense.

If you were to do that approach, then you could likely write the name property as

@property({ type: String, reflect: true })
public name?: string

I think this is more in line with our conventions of writing code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not so sure but I think name=${this.name ? this.name : undefined} will generate an error as shown in the below image
Screenshot 2023-11-10 at 9 25 01 AM


el = testFixture.querySelector('#checkbox1') as Checkbox;
expect(el.hasAttribute('name'));
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also include a test for the expected value of name if name = undefined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, what is the value of el.name here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, what is the value of el.name here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the value here is undefined because it's not yet initialised

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added 2 extra tests here for the same

@@ -237,6 +240,7 @@ export class TextfieldBase extends ManageHelpText(
: nothing}
<!-- @ts-ignore -->
<textarea
name=${ifDefined(this.name || undefined)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If name is a required attribute and has a value of an empty string no matter what, would this undefined state ever occur?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, I am using ifDefined because it renoves name attribute from dom(inspect element) when name is an empty string. If I don't use ifDefined then name attribute can be seen in inspect element section.
Screenshot 2023-11-10 at 9 52 02 AM

@@ -342,6 +342,37 @@ describe('Textfield', () => {
: null;
expect(input).to.not.be.null;
});
it('handles `name` attribute', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, curious to see a test where name is not specified by the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

Tests look good. I'm curious still what the property looks like if it's optional, ie

@property({ type: String, reflect: true }) 
public name?: string

Let me know if it behaves the same as what you have now.

el = testFixture.querySelector('#checkbox1') as Checkbox;
expect(el.hasAttribute('name'));
expect(el.name).to.be.undefined;
expect(el.name == undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
expect(el.name == undefined);

You can remove this line since it's the same as line 246.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember what Hunter found about this syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can remove this line since it's the same as line 246.

Done!

);
expect(el).to.not.equal(undefined);
expect(el.name).to.be.undefined;
expect(el.name === undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
expect(el.name === undefined);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

Thank you!

@najikahalsema najikahalsema merged commit 593005a into main Nov 14, 2023
46 of 47 checks passed
@najikahalsema najikahalsema deleted the blunteshwar/add-name-attribute-to-input-components branch November 14, 2023 17:49
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]: Name attribute for input components
3 participants