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: add slot for mt-data-table #210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

quando1910
Copy link

@quando1910 quando1910 commented Jun 4, 2024

What?

Refer this issue: #207

Why?

To have more chances for mt-data-table customization

How?

Just add some slots. Not change any logic.

Testing?

N/A

Screenshots (optional)

N/A

Anything Else?

N/A

Copy link

vercel bot commented Jun 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
meteor-component-library ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 4, 2024 1:32pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
meteor-admin-sdk-docs ⬜️ Ignored (Inspect) Visit Preview Jun 4, 2024 1:32pm

:column-definition="column"
@click="$emit('open-details', data)"
/>
<slot :name="`column-${column.property}`" v-bind="{ data, column }">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be insecure because this allows also overriding existing renderer. Better would be an approach like this:

<template v-else>
  <!-- Use the correct renderer for the column -->
  <mt-data-table-number-renderer
    v-if="column.renderer === 'number'"
    :data="data"
    :column-definition="column"
    @click="$emit('open-details', data)"
  />

  <mt-data-table-text-renderer
    v-else-if="column.renderer === 'text'"
    :data="data"
    :column-definition="column"
    @click="$emit('open-details', data)"
  />

  <mt-data-table-badge-renderer
    v-else-if="column.renderer === 'badge'"
    :data="data"
    :column-definition="column"
    @click="$emit('open-details', data)"
  />

  <mt-data-table-price-renderer
    v-else-if="column.renderer === 'price'"
    :data="data"
    :column-definition="column"
    @click="$emit('open-details', data)"
  />

  <slot name="custom-column-renderer" :v-bind="{ data, column }"></slot>
</template>

Then it is possible to add new, custom renderer which can be defined in the column definition. E.g.

<template #custom-column-renderer>
    <my-custom-renderer v-if="column.renderer ==== 'my-custom-renderer' />
</template>

Copy link
Author

@quando1910 quando1910 Jun 4, 2024

Choose a reason for hiding this comment

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

should we use v-else for slot ?

<slot v-else name="custom-column-renderer" v-bind="{ data, column }" />

I updated, please check again

@click="$emit('item-delete', data)"
/>
</mt-context-button>
<slot name="action-button" v-bind="{ data }">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. If someone uses this slot some logic with "disableEdit", etc. can get broken by using the slot wrong (e.g. the v-if before does not render the button even if the logic would make sense for it).

I would prefer to implement it in a more safe way with a slot just around the text. Then it is also customizable but is more stable.

<a v-if="!disableEdit" href="#" @click.prevent="$emit('open-details', data)">
  <slot name="action-button" v-bind="{ data }">
    {{ $t("mt-data-table.contextButtons.edit") }}
  </slot>
</a>

Copy link
Author

@quando1910 quando1910 Jun 4, 2024

Choose a reason for hiding this comment

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

by this way, we can not change the action for this button and can not style for it, for example, I want to replace by a "view details" button instead of the edit button. the case will be we just need another action totally, we don't do anything related to edit features

{{ $t("mt-data-table.contextButtons.edit") }}
</a>
</slot>
<slot name="context-button" v-bind="{ data }">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. This would allows to override the context buttons at a whole. I would prefer a property instead of a slot here. Something like this:

additionalContextButtons: {
    type: Array as PropType<Array<{
       // Here the needed information about the buttons
    }>>,
    required: false,
   default: undefined
}

Then we can iterate over this property in the template and can also adjust the v-if check for the context button:

<mt-context-button v-if="!(disableDelete && disableEdit) || additionalContextButtons?.length > 0">

And then do a v-for for all context buttons in the property

Copy link
Author

Choose a reason for hiding this comment

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

I updated, please check again

@@ -9,40 +9,48 @@
:model-value="searchValue"
@change="emitSearchValueChange"
/>
<template v-if="filters.length > 0">
Copy link
Contributor

Choose a reason for hiding this comment

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

@Haberkamp I think this implementation could be unstable very quickly. You are more into the topic of filters. Do you have a better, more defined approach for adding custom filter types?

I could imagine a general slot which allows adding custom "filter renderer"

Copy link
Author

@quando1910 quando1910 Jun 4, 2024

Choose a reason for hiding this comment

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

FYI, the current behavior is not proper with tablet, because it's open a popover, we want to customize the filter button, it will open the modal instead, and our specs requires price filter also, which is not support in popover for now. so we want to custom the button by our own, not reuse the popover logic.

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.

2 participants