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

Add prop(s) to hide popconfirm actions #770

Closed
spenserblack opened this issue Aug 3, 2021 · 6 comments · Fixed by #782
Closed

Add prop(s) to hide popconfirm actions #770

spenserblack opened this issue Aug 3, 2021 · 6 comments · Fixed by #782
Labels
feature request New feature or request

Comments

@spenserblack
Copy link
Contributor

This function solves the problem (这个功能解决的问题)

More consistency between Popconfirms and Popovers

Expected API (期望的 API)

<template>
  <n-popover>
    <template #trigger>
      <n-button> Hover </n-button>
    </template>
    <!-- NOTE because popovers can have a header, perhaps there should be a separate header icon slot -->
    <template #header-icon>
        <n-icon><!-- selected icon --></n-icon>
    </template>
    <template #icon>
        <n-icon><!-- selected icon --></n-icon>
    </template>
  <!-- popover text -->
  </n-popover>
</template>
@github-actions github-actions bot added the feature request New feature or request label Aug 3, 2021
@07akioni
Copy link
Collaborator

07akioni commented Aug 3, 2021

Won't do, they are two things.

If you need it it's very easy to encapsulate one.

@07akioni 07akioni closed this as completed Aug 3, 2021
@spenserblack
Copy link
Contributor Author

spenserblack commented Aug 3, 2021

🤔 To add an icon to a popover, simply using an <n-icon> doesn't seem to be enough. The margin isn't right.
I was able to decently replicate the icon spacing of n-popconfirm in n-popover with

<template>
<n-popover class="n-popconfirm" show>
  <template #trigger>I have a popover!</template>
  <div class="n-popconfirm__body">
    <div class="n-popconfirm__icon">
      <n-icon class="n-base-icon"><!-- icon --></n-icon>
    </div>
    Popover text
  </div>
</n-popover>
</template>

but this feels like a hack.

I can use CSS to add a margin, but is there another way to get the icon to look decent in a popover? Am I missing something that is already part of the API? I would like to use Naive UI's API and styling instead of custom styling as much as possible, to keep things consistent.

@07akioni
Copy link
Collaborator

07akioni commented Aug 4, 2021

I think we may provide a way to hide popconfirm's action.

Layout features can be endless, that's why I don't like to make layouts builtin unless they are quite common.

@spenserblack
Copy link
Contributor Author

I think we may provide a way to hide popconfirm's action.

This would help a lot! Should I change the title of this issue to "Add prop(s) to hide popconfirm actions"?


Just a thought, not a request:
If a popconfirm's actions can be hidden, doesn't this mean that a popconfirm can be made to behave exactly like a popover by setting trigger="hover"? It seems like popconfirm and popover could be combined into one component 🤔

@07akioni
Copy link
Collaborator

07akioni commented Aug 4, 2021

I think we may provide a way to hide popconfirm's action.

This would help a lot! Should I change the title of this issue to "Add prop(s) to hide popconfirm actions"?

Just a thought, not a request:
If a popconfirm's actions can be hidden, doesn't this mean that a popconfirm can be made to behave exactly like a popover by setting trigger="hover"? It seems like popconfirm and popover could be combined into one component 🤔

Yes I think you can do it.

@07akioni 07akioni reopened this Aug 4, 2021
@07akioni
Copy link
Collaborator

07akioni commented Aug 4, 2021

It seems like popconfirm and popover could be combined into one component

Actually they are nearly the same, popconfirm just wrap popover with some extra DOM.

It's possible to make it like

<n-popover>
  <template #trigger>xxx</template>
  <n-popconfirm-content>
    yyy
  </n-popconfirm-content>
</n-popover>

However I won't do that. It won't make user's DX better but add a lot of work in library development.

@spenserblack spenserblack changed the title Add icon slot for Popover Add prop(s) to hide popconfirm actions Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants