feat(textfield): added name attribute to textfield #3752
Conversation
Tachometer resultsChromecard permalink
checkbox permalink
number-field permalink
search permalink
slider permalink
switch permalink
table permalink
textfield permalink
Firefoxcard permalink
checkbox permalink
number-field permalink
search permalink
slider permalink
switch permalink
table permalink
textfield permalink
|
…github.com:adobe/spectrum-web-components into blunteshwar/add-name-attribute-to-input-components
| @@ -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; | |||
There was a problem hiding this comment.
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??
There was a problem hiding this comment.
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.
Also when I use public name: string = ''; I get the error shown above in image
There was a problem hiding this comment.
Gotcha. I think that's fair. However, would
@property({type: String, reflect: true})
public name?: string;
accomplish the same thing?
There was a problem hiding this comment.
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)} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am not so sure but I think name=${this.name ? this.name : undefined} will generate an error as shown in the below image
|
|
||
| el = testFixture.querySelector('#checkbox1') as Checkbox; | ||
| expect(el.hasAttribute('name')); | ||
| }); |
There was a problem hiding this comment.
Could you also include a test for the expected value of name if name = undefined?
There was a problem hiding this comment.
Also, what is the value of el.name here?
There was a problem hiding this comment.
Also, what is the value of el.name here?
There was a problem hiding this comment.
the value here is undefined because it's not yet initialised
There was a problem hiding this comment.
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)} | |||
There was a problem hiding this comment.
If name is a required attribute and has a value of an empty string no matter what, would this undefined state ever occur?
There was a problem hiding this comment.
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.
| @@ -342,6 +342,37 @@ describe('Textfield', () => { | |||
| : null; | |||
| expect(input).to.not.be.null; | |||
| }); | |||
| it('handles `name` attribute', async () => { | |||
There was a problem hiding this comment.
Again, curious to see a test where name is not specified by the user.
| el = testFixture.querySelector('#checkbox1') as Checkbox; | ||
| expect(el.hasAttribute('name')); | ||
| expect(el.name).to.be.undefined; | ||
| expect(el.name == undefined); |
There was a problem hiding this comment.
| expect(el.name == undefined); |
You can remove this line since it's the same as line 246.
There was a problem hiding this comment.
Remember what Hunter found about this syntax.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
| expect(el.name === undefined); |
najikahalsema
deleted the
blunteshwar/add-name-attribute-to-input-components
branch
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?
Screenshots (if appropriate)
Types of changes
Checklist
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.