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(VAutocomplete): divider/subheader not rendering #19912

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

func0der
Copy link

@func0der func0der commented May 27, 2024

Original PR: #15728

Description

This is just an updated version of #15728, since the functionality is still missing and the PR was closed as stale.
@nekosaur I hope this was okay?

You are welcome to update and fix accordingly.

I am not 100% in the Vuetify game and I might have made mistakes merging master. I am especially insecure about the changes in VTreeView, since the component was not present when the original PR was created.

Markup:

As described here: https://vuetifyjs.com/en/getting-started/contributing/#for-docs-language

<template>
  <v-card class="mx-auto" max-width="300">
    <v-list :items="items" />
    <v-select :items="items" />
    <v-autocomplete :items="items" />
    <v-treeview :items="items" />
  </v-card>
</template>

<script>
  export default {
    data: () => ({
      items: [
        {
          props: {
            subheader: true,
            divider: true,
          },
          title: 'Group #1',
          children: [
            {
              title: 'Item #1',
              value: 1,
            },
            {
              title: 'Item #2',
              value: 2,
            },
            {
              title: 'Item #3',
              value: 3,
            },
          ],
        },
        {
          props: {
            subheader: true,
          },
          title: 'Group #2',
          children: [
            {
              title: 'Item #4',
              value: 4,
            },
            {
              title: 'Item #5',
              value: 5,
            },
            {
              title: 'Item #6',
              value: 6,
            },
          ],
        },
      ],
    }),
  }
</script>

@func0der func0der changed the title Fix/15721 select list items fix(VAutocomplete): divider/subheader not rendering May 27, 2024
@func0der
Copy link
Author

func0der commented May 27, 2024

Known issues for now are:

  • Failing test because the rending of groups is somehow broken (have not found out why yet)
  • The syntax to define subheaders and dividers is unique to this usecase and different from the way of VListItem ,which would result in a breaking change and not a feature
  • There might still be unnecessary code in the like the !slots.default lines in VListItem that I can not quite place in the context of this issue

Copy link
Author

Choose a reason for hiding this comment

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

i don't know why this was deleted in the first place. MAybe @nekosaur can say something about that.

@johnleider johnleider added T: bug Functionality that does not work as intended/expected C: VAutocomplete VAutocomplete labels May 28, 2024
@func0der
Copy link
Author

@yuwu9145 @johnleider @KaelWD
Any chance we can get a review on this, so I can work on possible change requests and we can continue to work on this? :)

Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

What is the reasoning from switching from InternalListItem to ListItem?

@@ -294,13 +294,13 @@ export const VListItem = genericComponent<VListItemSlots>()({
)}

<div class="v-list-item__content" data-no-activator="">
{ hasTitle && (
{ hasTitle && !slots.default && (
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

Actually. I have no real clue. This is something I just copied over from the other PR. It came with this commit, which I think is not really related to this change: e6792c1

If it is not needed or is bad practice, I will remove it.

<VListItemTitle key="title">
{ slots.title?.({ title: props.title }) ?? props.title }
</VListItemTitle>
)}

{ hasSubtitle && (
{ hasSubtitle && !slots.default && (
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

Actually. I have no real clue. This is something I just copied over from the other PR. It came with this commit, which I think is not really related to this change: e6792c1

If it is not needed or is bad practice, I will remove it.

packages/vuetify/src/components/VSelect/VSelect.tsx Outdated Show resolved Hide resolved
packages/vuetify/src/composables/list-items.ts Outdated Show resolved Hide resolved
@func0der
Copy link
Author

@nekosaur Would love for you to chip in on some of those review comments/question, if you can :)

@func0der
Copy link
Author

What is the reasoning from switching from InternalListItem to ListItem?

I think that the InternalListItem was more of a helper construct inside the component and not something that was actually used.
Since the whole list flattening is now inside list-items.ts it is not needed anymore. Inside list-items.ts the type is not called FlatListItem.

@johnleider johnleider requested a review from nekosaur June 12, 2024 13:47
@@ -87,7 +88,9 @@ export const makeSelectProps = propsFactory({
openOnClear: Boolean,
itemColor: String,

...makeItemsProps({ itemChildren: false }),
...makeItemsProps({
itemProps: () => (item: any) => ({ subheader: item.subheader, divider: item.divider }),

Choose a reason for hiding this comment

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

I tested the branch with the provided markup and the dividers are missing for the VSelect (and VAutocomplete). It seems like this is expecting subheader and divider to be at the root, but it is not the case.

image

Looking into item.props?.subheader works. It also works like that:

Suggested change
itemProps: () => (item: any) => ({ subheader: item.subheader, divider: item.divider }),
itemProps: 'props',

image

Choose a reason for hiding this comment

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

Unless the markup you provided is not the good one @func0der ? I see on the original PR that subheader and divider were at the root of the item

@gduliscouet-ubitransport

We are quite interested in this feature and I would love to help moving this PR forward. But it is, I think, too much to handle safely by people unfamilliar with the Vuetify codebase. Would a core member be able to help on this ? @johnleider @KaelWD ?

@johnleider
Copy link
Member

We are quite interested in this feature and I would love to help moving this PR forward. But it is, I think, too much to handle safely by people unfamilliar with the Vuetify codebase. Would a core member be able to help on this ? @johnleider @KaelWD ?

I've asked @nekosaur if he could take a look at some point when he has time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VAutocomplete VAutocomplete T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants