Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Add label to the autocomplete component #1156

Merged
merged 2 commits into from
Mar 18, 2021
Merged

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Mar 18, 2021

Adding the label "from the outside" is not a11y compliant as you cannot get the id of the actual input element which therefore cannot be referenced via for. This is a breaking change though as I renamed aria-label to label. We could leave it, but technically it's not aria anymore now.

This change also removes all the unnecessary prefixes which were used in the component.

Needed for owncloud/web#4329:

"Personen hinzufügen" is a <label>, but points (via for) to a <div> instead of an input.

@JammingBen JammingBen requested a review from LukasHirt as a code owner March 18, 2021 10:03
@JammingBen JammingBen self-assigned this Mar 18, 2021
@JammingBen JammingBen added accessibility enhancement New feature or request labels Mar 18, 2021
Copy link

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

Nice 🚀

@pascalwengerter
Copy link
Contributor

Breaking change should be fine (since my theming PR breaks some stuff anyways and we'll have to release;)

@kulmann
Copy link
Member

kulmann commented Mar 18, 2021

Would it be possible to use the oc-text-input component in here? Because in this case we actually want to display a form input...

@JammingBen
Copy link
Contributor Author

Would it be possible to use the oc-text-input component in here? Because in this case we actually want to display a form input...

We could do that, yeah. A few thinks that would mean:

  • No more placeholders. Those will be the replaced by the description below the input.
  • As a result, the explicit aria-describedby property will also be removed.
  • Some (small) styling adjustments are needed (because the result div should be displayed directly below the input instead of below the description:):

image

@kulmann
Copy link
Member

kulmann commented Mar 18, 2021

Would it be possible to use the oc-text-input component in here? Because in this case we actually want to display a form input...

We could do that, yeah. A few thinks that would mean:

  • No more placeholders. Those will be the replaced by the description below the input.
  • As a result, the explicit aria-describedby property will also be removed.
  • Some (small) styling adjustments are needed (because the result div should be displayed directly below the input instead of below the description:):

All the attributes you assign to a oc-text-input will be applied to its input, because we have a v-bind with everything that comes from $attrs. That includes placeholders and aria-describedby, you can just set them on the oc-text-input and they will magically appear on the input. :-)

But still, I'm fine with keeping the dedicated input. Could you copy the styles of the oc-text-input and apply them to the input here as well? Would be nice if all inputs had the same look and feel + theming.

@JammingBen
Copy link
Contributor Author

JammingBen commented Mar 18, 2021

All the attributes you assign to a oc-text-input will be applied to its input, because we have a v-bind with everything that comes from $attrs. That includes placeholders and aria-describedby, you can just set them on the oc-text-input and they will magically appear on the input. :-)

Ahh yes, you're right :) aria-describedby however has its own "logic" in the OcTextInput component, which is connected to the description (or errors if there are any). That might still clash in specific scenarios.

But still, I'm fine with keeping the dedicated input. Could you copy the styles of the oc-text-input and apply them to the input here as well? Would be nice if all inputs had the same look and feel + theming.

I actually like the idea of using OcTextInput here, but again, it's probably easier with a custom input... I digged into it and faced another issue where we would need a (<div>-)element to attach the dropdown to (in between the input and the description).

@kulmann
Copy link
Member

kulmann commented Mar 18, 2021

Ahh yes, you're right :) aria-describedby however has its own "logic" in the OcTextInput component, which is connected to the description (or errors if there are any). That might still clash in specific scenarios.

True :-) Didn't think about that.

I actually like the idea of using OcTextInput here, but again, it's probably easier with a custom input... I digged into it and faced another issue where we would need a (<div>-)element to attach the dropdown to (in between the input and the description).

Yep, makes sense. :-) proper styling for the custom input is good 👍

@JammingBen
Copy link
Contributor Author

Alrighty, done!

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

😍

@kulmann kulmann merged commit a59ab78 into master Mar 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the issues/autocomplete-label branch March 18, 2021 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants