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

add default name attr to input as placeholder #20

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

napafundi
Copy link
Contributor

No description provided.

@napafundi napafundi requested review from bakerac4 and billdami March 12, 2021 17:57
@@ -20,6 +20,7 @@
@value={{@value}}
class="{{this.inputBaseClass}}"
id={{this.id}}
name={{or @name @placeholder}}
Copy link
Member

Choose a reason for hiding this comment

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

@napafundi is this for a11y concerns? My only question is generally name attributes are "code friendly" names, e.g. no spaces/punctuation ect, so using the placeholder value might not always be best, its possible we want to sanitize the placeholder (replace spaces w/ underscores, etc) before using it here. But, might not be the end of the world.

@@ -50,6 +52,11 @@ export default class FlInput<T extends FlInputArgs> extends Component<T> {
return this.args.id ?? `fl-input-${guidFor(this)}`;
}

get inputName() {
const name = this.args.name ?? this.args.placeholder;
return name ? dasherize(name) : '';
Copy link
Member

Choose a reason for hiding this comment

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

prob shouldnt touch a manually passed in name arg. e.g.

get inputName() {
    return this.args.name ?? dasherize(this.args.placeholder);
}

also i forget, but does dasherize() handle stripping out non alphanumeric symbols for you too? e.g. for like "Bob's Payment Amount ($)" => "bobs-payment-amount"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out it doesn't. Updated that to remove non-word characters as ^ ended up as "bob's-payment-amount-($)"

@@ -40,6 +41,11 @@ export default class FlSelect extends FlInput<FlSelectArgs> {
return !this.allowClear;
}

get selectName() {
Copy link
Member

Choose a reason for hiding this comment

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

id probably rename the getter in FlInput to something more generic like just get name(), so that you don't have to reimplement in here or FlTextarea, since both extend FlInput.

@napafundi napafundi requested a review from billdami March 12, 2021 19:26
@@ -50,6 +52,11 @@ export default class FlInput<T extends FlInputArgs> extends Component<T> {
return this.args.id ?? `fl-input-${guidFor(this)}`;
}

get name() {
const name = this.args.name ?? this.args.placeholder;
return name ? dasherize(name.replace(/\W/g, '')) : '';
Copy link
Member

Choose a reason for hiding this comment

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

looks good i just wouldn't muck with a manually passed in @name arg value, i'd do it like:

get name() {
    return this.args.name ?? dasherize(this.args.placeholder?.replace(/\W/g, '') ?? '');
}

@napafundi napafundi requested a review from billdami March 12, 2021 19:32
@napafundi napafundi merged commit 20d7549 into master Mar 12, 2021
@billdami billdami deleted the feature/tp66923-accessibility branch March 12, 2021 19:43
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.

2 participants