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

feat(SelectMenu): add selected value to label slot data #1349

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

CernyMatej
Copy link
Contributor

@CernyMatej CernyMatej commented Feb 12, 2024

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Currently, it is not possible to access the selected value in the label slot.
This PR adds it to the slot data of the label slot.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@benjamincanac
Copy link
Member

Not sure to understand the point of exposing the modelValue. If you do a v-model="selected", you already have access to the selected variable.

@CernyMatej CernyMatej marked this pull request as draft February 13, 2024 10:01
@CernyMatej
Copy link
Contributor Author

CernyMatej commented Feb 13, 2024

The purpose of this change was to allow access to the original object (value) when the value-attribute prop is set. However, after reading your comment, I realized that the initial implementation was pointless. πŸ™ˆ I have made the following updates:

  1. Fixed a bug where the HListbox component would throw an error upon being clicked if the multiple prop was set and modelValue was not an array type. (added a fallback & logged a warning to the console because it wasn't obvious what the problem was from the error message at first glance)
  2. Corrected an issue where the label displayed an incorrect number of selected items (when the multiple prop was set) if the modelValue array contained an item not present in the options.
  3. Added the actual selected value to the label slot.

However, there are a few points for discussion. The :selected attribute in the label slot has a different meaning than in other slots (indicating an active option versus whether the option is selected). Should we consider renaming it in the label slot to avoid confusion?

Also, when the multiple prop is set, the modelValue array will reflect the items in the order they were clicked by the user. However, in the label slot, the selected values will appear in the same order as in the options prop.

(I'll add documentation after we resolve the points above)

@@ -355,17 +359,34 @@ export default defineComponent({
}
})

const selected = computed(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure to understand why you added this computed, could you provide a real-life example where you need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!
When you change the selected value using the value-attribute prop, there is currently no way (at least as far as I'm aware) to access the remaining attributes of the selected object.
Let's say we have an array of categories with translated names:

const categories = [
  {
    id: 1,
    name: {
      en: 'Category',
      de: 'Kategorie',
      // ...
    },
  },
  // ...
]

This is how I would use it:

<USelectMenu
    :options="categories"
    value-attribute="id"
>
    <template #label="{ selected }">
        {{ useLocalizedValue(selected.name) }}
    </template>
</USelectMenu>

This applies to the #leading and #trailing slots as well. Currently, you wouldn't be able to access any icon attributes of the object in those slots, if you changed the value-attribute, would you? So it might be worth adding this to those slots as well.

<!--  example from the docs  -->
<template>
  <USelectMenu v-model="selected" :options="people"> <!--  if we add `value-attribute="id"`, this won't work  --> 
    <template #leading>
      <UIcon v-if="selected.icon" :name="(selected.icon as string)" class="w-4 h-4 mx-0.5" />
      <UAvatar v-else-if="selected.avatar" v-bind="(selected.avatar as Avatar)" size="3xs" class="mx-0.5" />
    </template>
  </USelectMenu>
</template>

Or am I missing something?

@lammerfalcon
Copy link
Contributor

is there a solution to show selected.name in label slot, when i override value-attribute?

@CernyMatej
Copy link
Contributor Author

@lammerfalcon this is exactly what this PR is meant to solve

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.

None yet

3 participants