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

fix(SelectMenu): use by prop to compare objects for selected values #2228

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

Conversation

CernyMatej
Copy link
Contributor

@CernyMatej CernyMatej commented Sep 20, 2024

πŸ”— Linked issue

fixes #2028

❓ 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

This PR fixes a bug introduced in #1349, where the by prop was not being consiederd when computing the selected values.
It also adds the possibility to combine the by prop with the value-attribute prop to compare nested objects like in the following example:

<template>
  <USelectMenu
    v-model="selected"
    :options="options"
    multiple
    value-attribute="object"
    by="id"
  />
</template>

<script lang="ts" setup>
const selected = ref([{
  id: 1,
  title: 'Hello'
}])

const options = [
  {
    key: 'id',
    label: 'ID',
    object: {         // πŸ‘ˆ this will be the selected value
      id: 1,             // πŸ‘ˆ the objects will be compared by this attribute
      title: 'Hello'
    }
  },
  {
    key: 'title',
    label: 'Title',
    object: {
      id: 2,             // πŸ‘ˆ the objects will be compared by this attribute
      title: 'Hi'
    }
  }
]
</script>

πŸ“ Checklist

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

@@ -481,7 +490,7 @@ export default defineComponent({
}

return props.options || []
}, [], {
}, props.options || [], {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjamincanac Since the selected computed proeprty uses the computedAsync optionsΒ to get the possible values, I initialized it to props.optionsΒ so that it isn't empty during the initial render of the component to prevent a flash.
However, now that I'm thinking about it, shouldn't we just use props.options directly in the selectedΒ computed, instead of options - since we don't want to compare the model values with only the search results, but rather the whole options?

Copy link
Contributor Author

@CernyMatej CernyMatej Sep 20, 2024

Choose a reason for hiding this comment

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

I used options because that's what was used previously, however, unless I'm missing something, I think that props.options should have been used there as well:

image

Though this only affects things if someone uses asynchronous search.

@CernyMatej
Copy link
Contributor Author

I also added the possibility to compare nested objects using the by prop when the value-attribute prop is set

Consider the following example:

<template>
  <USelectMenu
    v-model="selected"
    :options="options"
    multiple
    value-attribute="object"
    by="id"
  />
</template>

<script lang="ts" setup>
const selected = ref([{
  id: 1,
  title: 'Hello'
}])

const options = [
  {
    key: 'id',
    label: 'ID',
    object: {
      id: 1,
      title: 'Hello'
    }
  },
  {
    key: 'title',
    label: 'Title',
    object: {
      id: 2,
      title: 'Hi'
    }
  },
  {
    key: 'status',
    label: 'Status',
    object: {
      id: 3,
      title: 'Hey'
    }
  }
]
</script>

Previously (even before the introduction of the selected computed) it was not possible to combine these 2 props. This approach made sense to me, but I’m happy to hear any feedback on this.

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.

USelectMenu multiple count error
1 participant