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.

Sign up for GitHub

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

Jump to bottom

feat(NcListItem): provide additional slots for NcActions / extra actions customization #5870

Merged
merged 1 commit into from
Aug 3, 2024
Merged

feat(NcListItem): provide additional slots for NcActions / extra actions customization #5870

merged 1 commit into from
Aug 3, 2024

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jul 25, 2024 •
edited
Loading

☑️ Resolves

🖼️ Screenshots

image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@Antreesy Antreesy added bug Something isn't working 3. to review Waiting for reviews feature: list-item Related to the list-item component labels Jul 25, 2024
@Antreesy Antreesy added this to the 8.15.1 milestone Jul 25, 2024
@Antreesy Antreesy requested a review from ShGKme July 25, 2024 12:47
@Antreesy Antreesy self-assigned this Jul 25, 2024
@Antreesy Antreesy mentioned this pull request Jul 25, 2024
Open
23 tasks
@Antreesy Antreesy requested a review from susnux July 25, 2024 18:29
@Antreesy Antreesy mentioned this pull request Jul 25, 2024
Merged
5 tasks
susnux
susnux reviewed Jul 25, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Not really sure, either we:

  1. allow any actions, then there should be only one slot and we deprecate the actions slot
  2. Only allow actions like currently, but then probably we need to allow at least an icon slot

@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 25, 2024 •
edited
Loading

2. we need to allow at least an icon slot

And something like an actionProps, like inline e.t.c
But I'd agree, that option 1 is more durable. maybe we can support both for v8 and drop starting from v9?

@susnux
Copy link
Contributor

susnux commented Jul 25, 2024

Thats why I would go with 1 for the future, no?

@susnux susnux added enhancement New feature or request and removed bug Something isn't working labels Jul 29, 2024
@susnux susnux changed the title fix(NcListItem): allow to pass custom NcActions component via scoped slot feat(NcListItem): allow to pass custom NcActions component via scoped slot Jul 29, 2024
@susnux susnux modified the milestones: 8.15.1, 8.15.2, 8.16.0 Jul 29, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Aug 2, 2024

  1. allow any actions, then there should be only one slot and we deprecate the actions slot

Then it is more complex to use because we need to define some props and events of actions

@ShGKme
Copy link
Contributor

ShGKme commented Aug 2, 2024

@susnux What about another way:

  1. Add actions-icon slot just for icon
  2. Add new extra-actions slot for additional buttons.

So any additional buttons are added not via inline options but via a new slot as a normal buttons?

@susnux
Copy link
Contributor

susnux commented Aug 2, 2024

@susnux What about another way:

1. Add `actions-icon` slot just for icon

This I think makes really sense!

2. Add new `extra-actions` slot for additional buttons.

Do you have a use case for this on Talk? Otherwise I think it is not a good idea to have multiple buttons here.
But would be also ok with this.

@Antreesy Antreesy force-pushed the fix/noid/list-item-custom-actions branch from 4300707 to 4ea068a Compare August 3, 2024 07:57
@Antreesy Antreesy changed the title feat(NcListItem): allow to pass custom NcActions component via scoped slot feat(NcListItem): provide additional slots for NcActions / extra actions customization Aug 3, 2024
@Antreesy Antreesy requested a review from susnux August 3, 2024 07:58
@Antreesy Antreesy force-pushed the fix/noid/list-item-custom-actions branch from 4ea068a to 8aefc8e Compare August 3, 2024 08:02
@ShGKme
Copy link
Contributor

ShGKme commented Aug 3, 2024

Do you have a use case for this on Talk?

We have almost exactly the same component re-implemented for participants in Talk. We have additional buttons there in a lobby and with SIP:

image

ShGKme
ShGKme approved these changes Aug 3, 2024
View reviewed changes
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
@susnux
Copy link
Contributor

susnux commented Aug 3, 2024

We have almost exactly the same component re-implemented for participants in Talk. We have additional buttons there in a lobby and with SIP:

Ah ok makes sense.

(OT: I really do not like this component it is so complex to fit every use case even if it would be simpler to just create a just custom component for it, while most often use case is just"a simple listitem").

susnux
susnux approved these changes Aug 3, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Please apply the comment by @ShGKme (template v-if) otherwise fine

@Antreesy
feat(NcListItem): provide additional slots for customization
2d26aad 
- extra-actions for inline component
- actions-icon for icon in NcActions component

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/list-item-custom-actions branch from 8aefc8e to 2d26aad Compare August 3, 2024 14:00
@Antreesy Antreesy merged commit dafcc8e into master Aug 3, 2024
19 checks passed
@Antreesy Antreesy deleted the fix/noid/list-item-custom-actions branch August 3, 2024 14:06
@Antreesy
Copy link
Contributor Author

Antreesy commented Aug 3, 2024

/backport to next

@backportbot backportbot bot mentioned this pull request Aug 3, 2024
Merged
2 tasks
@Antreesy Antreesy mentioned this pull request Aug 5, 2024
Merged
@Stanislav1975 Stanislav1975 mentioned this pull request Aug 26, 2024
Open
This was referenced Sep 6, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@ShGKme ShGKme ShGKme approved these changes

Assignees

@Antreesy Antreesy

Labels
3. to review Waiting for reviews enhancement New feature or request feature: list-item Related to the list-item component
Projects
None yet
Milestone
8.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Antreesy @susnux @ShGKme