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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Nov 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13524d4
feat(textfield): added name attribute to textfield
blunteshwar Oct 31, 2023
2cdbde5
Merge branch 'main' into blunteshwar/add-name-attribute-to-input-comp…
blunteshwar Nov 7, 2023
3ba420c
chore(checkbox,switch): added name attribute
blunteshwar Nov 9, 2023
9cf2085
Merge branch 'main' into blunteshwar/add-name-attribute-to-input-comp…
blunteshwar Nov 9, 2023
2b6d470
chore(textfield): updated golden image hash
blunteshwar Nov 9, 2023
fcd0b28
Merge branch 'blunteshwar/add-name-attribute-to-input-components' of …
blunteshwar Nov 9, 2023
0066dd1
chore(textfield,checkbox): suggested changes
blunteshwar Nov 10, 2023
2e07fe0
chore(textfield,checkbox): suggested chnages
blunteshwar Nov 14, 2023
6006ddd
Merge branch 'main' into blunteshwar/add-name-attribute-to-input-comp…
blunteshwar Nov 14, 2023
180a117
Merge branch 'main' into blunteshwar/add-name-attribute-to-input-comp…
najikahalsema Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ executors:
parameters:
current_golden_images_hash:
type: string
default: 51f40cf6afe7adcbfc314c6f9341fbb48f4cbe98
default: 9cf2085d96fcad2e79e07df8fdf6a8d74988d731
wireit_cache_name:
type: string
default: wireit
Expand Down
5 changes: 5 additions & 0 deletions packages/checkbox/src/CheckboxBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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).


@query('#input')
protected inputElement!: HTMLInputElement;

Expand Down Expand Up @@ -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

id="input"
type="checkbox"
.checked=${this.checked}
Expand Down
11 changes: 10 additions & 1 deletion packages/checkbox/test/checkbox.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
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


it('handles click events', async () => {
const el = testFixture.querySelector('#checkbox1') as Checkbox;
expect(el.checked).to.be.true;
Expand Down Expand Up @@ -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 () => {
Expand Down
11 changes: 11 additions & 0 deletions packages/switch/test/switch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ describe('Switch', () => {

expect(labelEl.getAttribute('for')).to.equal(inputEl.id);
});
it('has name attribute', async () => {
const el = await fixture<Switch>(
html`
<sp-switch>Not Checked</sp-switch>
`
);

await elementUpdated(el);

await expect(el.hasAttribute('name'));
});
it('loads `checked` switch accessibly', async () => {
const el = await fixture<Switch>(
html`
Expand Down
5 changes: 5 additions & 0 deletions packages/textfield/src/Textfield.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ export class TextfieldBase extends ManageHelpText(
@property()
public label = '';

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

@property()
public placeholder = '';

Expand Down Expand Up @@ -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

aria-describedby=${this.helpTextId}
aria-label=${this.label ||
this.appliedLabel ||
Expand Down Expand Up @@ -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 ||
Expand Down
10 changes: 10 additions & 0 deletions packages/textfield/stories/textfield.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ export const allowedKeys = (): TemplateResult => {
`;
};

export const withNameAttribute = (): TemplateResult => {
return html`
<sp-textfield
name="name"
placeholder="Enter your name"
allowed-keys="a-z"
></sp-textfield>
`;
};

export const readonly = (): TemplateResult => html`
<sp-textfield
label="Enter your life story"
Expand Down
28 changes: 28 additions & 0 deletions packages/textfield/test/textfield.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,34 @@ 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!

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`
Expand Down