Add label to OcSelect #1570

pascalwengerter · 2021-08-05T15:23:24Z

Description

See changelog item

pascalwengerter · 2021-08-05T15:23:54Z

Cherrypicking this commit to master will close #1503

kulmann · 2021-08-16T15:20:14Z

src/components/OcSelect.vue

@@ -32,6 +35,11 @@ export default {
      required: false,
      default: null,
    },
+    labelText: {


Would it make sense to bind $attrs without the label prop to the vue-select? So that we can have a dedicated label prop on the OcSelect? I kind of dislike using labelText as prop name here, while we use label on all other form related elements.

Full ack! Streamline all the things 🥳

pascalwengerter · 2021-08-31T15:40:36Z

@kulmann ready for re-review. Feels a bit hacky and prevents us from using the vue-select label prop but gets the job done

lookacat · 2021-09-02T08:18:14Z

src/components/OcSelect.vue

@@ -33,6 +42,11 @@ export default {
      default: null,
    },
  },
+  computed: {
+    setLabel() {


i find this somehow misleading, sounds like a method. Maybe selectLabel?

How about inputLabel? @kulmann any suggestions/opinions?

yeah, inputLabel is fine

kulmann · 2021-09-02T12:19:48Z

src/components/OcSelect.vue

@@ -42,6 +56,11 @@ export default {
      const comboBoxElement = this.$refs.select.$el.querySelector("div:first-child")
      comboBoxElement.setAttribute("aria-label", this.$gettext("Search for option"))
    },
+    setCustomizedAttrs() {


This is not setting anything. It should be a computed prop (and have a different name, like customizedAttrs or something like that)

Also, you should copy the this.$attrs first before modifying.

dschmidt · 2021-09-03T08:09:52Z

changelog/unreleased/enhancement-ocselect-label

@@ -0,0 +1,5 @@
+Enhancement: Label for OcSelect
+
+We've added a configurable `<label>` for the OcSelect label.


Tbh this sounds a bit strange.

You've just added a label for OcSelect, no?

dschmidt · 2021-09-03T08:12:50Z

changelog/unreleased/enhancement-ocselect-label

+
+We've added a configurable `<label>` for the OcSelect label.
+
+https://github.com/owncloud/web/issues/1570


Should be #1503 I think

Ah, nope - #1570 hehe

Number was right, just the wrong repo

dschmidt

IMHO this is a breaking change (I'm using the label property already) and should be marked as such in the changelog.
After all you had to change the documentation/example because it was broken ;-)

I would prefer having an option-label prop instead of having to use the option slot but that's obviously debatable.

dschmidt · 2021-09-03T08:17:41Z

src/components/OcSelect.vue

+Sometimes we need to display more complex options. This can include e.g. an option with a title and a description. 
+To still display all those values exactly as we want to, we need to use scoped slot called `option`.
+We can then retrieve all the values that we want to display from the slots parametres. 
+It is important to use a `label` key inside the passed object since `oc-select` will look for it.


This sounds rather complicated. Why don't we introduce an option-label property that will be passed as label to vue-select?

Sounds like a great idea actually, let me take a look 🕵🏽

Or get it from $attrs fwiw

dschmidt · 2021-09-03T08:22:51Z

In general: yay!
With this we can replace one more ❌ with ✔️ in owncloud/web#8857 :-D
Much appreciated if you could do that once this gets merged.

dschmidt · 2021-09-03T11:03:26Z

src/components/OcSelect.vue

-      return this.$attrs["label"]
+    /**
+     * Label of the select component
+     * ATTENTION: this shadows the vue-select option `label`. If you need access to that use `optionLabel`.


It shadows the vue select "prop"

dschmidt · 2021-09-03T16:29:45Z

src/components/OcSelect.vue

-    <div slot="no-options" v-translate>No options available.</div>
-  </vue-select>
+  <div>
+    <label :for="this.$attrs['input-id']" v-text="label" />


Where is this supposed to come from? How is this wired up with vue-select?

Ah I assume vue-select has this prop? In other places we seem to use an id prop, maybe we should stick to that

https://vue-select.org/api/props.html#inputid

owncloud-design-system/src/components/OcTextInput.vue

Line 70 in 17e7e6f

/**

dschmidt · 2021-09-04T20:48:00Z

CI fails because of unit tests, looks completely unrelated to me. OcSelect does not have unit tests after all

pascalwengerter · 2021-09-06T14:26:40Z

Squash-merging this into the a11y-updates branch now as it has the same changeset as #1633 (@dschmidt your review was implemented, thanks for the constructive feedback and collaboration!)

pascalwengerter requested a review from LukasHirt as a code owner August 5, 2021 15:23

pascalwengerter requested review from kulmann and removed request for LukasHirt August 5, 2021 15:26

kulmann reviewed Aug 16, 2021

View reviewed changes

pascalwengerter force-pushed the label_for_ocSelect branch from eb13580 to 69140e6 Compare August 31, 2021 15:39

pascalwengerter requested a review from kulmann August 31, 2021 15:39

Add label to OcSelect

f889be9

pascalwengerter force-pushed the label_for_ocSelect branch from 69140e6 to f889be9 Compare August 31, 2021 15:48

lookacat reviewed Sep 2, 2021

View reviewed changes

kulmann reviewed Sep 2, 2021

View reviewed changes

dschmidt reviewed Sep 3, 2021

View reviewed changes

dschmidt suggested changes Sep 3, 2021

View reviewed changes

dschmidt mentioned this pull request Sep 3, 2021

Streamline props and events for all form components owncloud/web#8857

Open

dschmidt reviewed Sep 3, 2021

View reviewed changes

dschmidt force-pushed the label_for_ocSelect branch from e881e5d to c4cbb27 Compare September 4, 2021 20:50

dschmidt mentioned this pull request Sep 4, 2021

Add label to OcSelect (port to master) #1633

Merged

10 tasks

dschmidt force-pushed the label_for_ocSelect branch 2 times, most recently from bd6b7da to ee9c5b6 Compare September 4, 2021 21:27

OcSelect: Add optionLabel prop which is passed as label to vue-select

628e848

dschmidt force-pushed the label_for_ocSelect branch from ee9c5b6 to 628e848 Compare September 4, 2021 22:16

Move input-id into additional attributes computed prop

365ec6a

pascalwengerter merged commit 088159b into a11y-updates Sep 6, 2021

delete-merged-branch bot deleted the label_for_ocSelect branch September 6, 2021 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add label to OcSelect #1570

Add label to OcSelect #1570

pascalwengerter commented Aug 5, 2021

pascalwengerter commented Aug 5, 2021

kulmann Aug 16, 2021

dschmidt Sep 3, 2021

pascalwengerter commented Aug 31, 2021

lookacat Sep 2, 2021

pascalwengerter Sep 2, 2021

kulmann Sep 2, 2021

kulmann Sep 2, 2021

kulmann Sep 2, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021 •

edited

Loading

dschmidt left a comment •

edited

Loading

dschmidt Sep 3, 2021 •

edited

Loading

pascalwengerter Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt commented Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt Sep 3, 2021

dschmidt commented Sep 4, 2021

pascalwengerter commented Sep 6, 2021

		@@ -0,0 +1,5 @@
		Enhancement: Label for OcSelect

		We've added a configurable `<label>` for the OcSelect label.


		We've added a configurable `<label>` for the OcSelect label.

		https://github.com/owncloud/web/issues/1570

Add label to OcSelect #1570

Add label to OcSelect #1570

Conversation

pascalwengerter commented Aug 5, 2021

Description

pascalwengerter commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalwengerter commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschmidt Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

dschmidt left a comment • edited Loading

Choose a reason for hiding this comment

dschmidt Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschmidt commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschmidt commented Sep 4, 2021

pascalwengerter commented Sep 6, 2021

dschmidt Sep 3, 2021 •

edited

Loading

dschmidt left a comment •

edited

Loading

dschmidt Sep 3, 2021 •

edited

Loading