-
Notifications
You must be signed in to change notification settings - Fork 192
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
Changes from all commits
13524d4
2cdbde5
3ba420c
9cf2085
2b6d470
fcd0b28
0066dd1
2e07fe0
6006ddd
180a117
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { | |
property, | ||
query, | ||
} from '@spectrum-web-components/base/src/decorators.js'; | ||
import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; | ||
import { Focusable } from '@spectrum-web-components/shared/src/focusable.js'; | ||
|
||
export class CheckboxBase extends Focusable { | ||
|
@@ -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; | ||
|
||
@query('#input') | ||
protected inputElement!: HTMLInputElement; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing on here, could we do something like
instead? I think wrapping If you were to do that approach, then you could likely write the
I think this is more in line with our conventions of writing code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
id="input" | ||
type="checkbox" | ||
.checked=${this.checked} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,6 +238,16 @@ describe('Checkbox', () => { | |
expect(el.checked).to.be.true; | ||
}); | ||
|
||
it('has name attribute', () => { | ||
let el = testFixture.querySelector('#checkbox0') as Checkbox; | ||
|
||
el = testFixture.querySelector('#checkbox1') as Checkbox; | ||
expect(el.hasAttribute('name')); | ||
expect(el.name).to.be.undefined; | ||
el.setAttribute('name', 'test'); | ||
expect(el.name).to.be.equal('test'); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also include a test for the expected value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, what is the value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, what is the value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the value here is undefined because it's not yet initialised There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added 2 extra tests here for the same |
||
|
||
it('handles click events', async () => { | ||
const el = testFixture.querySelector('#checkbox1') as Checkbox; | ||
expect(el.checked).to.be.true; | ||
|
@@ -297,7 +307,6 @@ describe('Checkbox', () => { | |
expect(el.indeterminate).to.be.false; | ||
expect(inputEl.checked).to.be.false; | ||
expect(inputEl.indeterminate).to.be.false; | ||
|
||
}); | ||
|
||
it('`indeterminate, not checked` becomes `checked` on click', async () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,9 @@ export class TextfieldBase extends ManageHelpText( | |
@property() | ||
public label = ''; | ||
|
||
@property({ type: String, reflect: true }) | ||
public name: string | undefined; | ||
|
||
@property() | ||
public placeholder = ''; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
aria-describedby=${this.helpTextId} | ||
aria-label=${this.label || | ||
this.appliedLabel || | ||
|
@@ -270,6 +274,7 @@ export class TextfieldBase extends ManageHelpText( | |
return html` | ||
<!-- @ts-ignore --> | ||
<input | ||
name=${ifDefined(this.name || undefined)} | ||
type=${this.type} | ||
aria-describedby=${this.helpTextId} | ||
aria-label=${this.label || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -342,6 +342,34 @@ describe('Textfield', () => { | |
: null; | ||
expect(input).to.not.be.null; | ||
}); | ||
it('handles `name` attribute', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, curious to see a test where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
const el = await litFixture<Textfield>( | ||
html` | ||
<sp-textfield placeholder="Enter your name"></sp-textfield> | ||
` | ||
); | ||
expect(el).to.not.equal(undefined); | ||
expect(el.name).to.be.undefined; | ||
|
||
el.setAttribute('name', 'test'); | ||
expect(el.name).to.be.equal('test'); | ||
}); | ||
it('handles `name` attribute with multiline', async () => { | ||
const el = await litFixture<Textfield>( | ||
html` | ||
<sp-textfield | ||
name="name" | ||
placeholder="Enter your name" | ||
multiline | ||
></sp-textfield> | ||
` | ||
); | ||
expect(el).to.not.equal(undefined); | ||
const input = el.shadowRoot | ||
? el.shadowRoot.querySelector('textarea') | ||
: null; | ||
expect(input?.name).to.equal('name'); | ||
}); | ||
it('valid - multiline', async () => { | ||
const el = await litFixture<Textfield>( | ||
html` | ||
|
There was a problem hiding this comment.
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 thename
attribute, then we want to explicitly state thatname = undefined
, right?^ 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.
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;
andpublic 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 writepublic name: string = '';
instead ofpublic 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 imageThere was a problem hiding this comment.
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
accomplish the same thing?
There was a problem hiding this comment.
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).