Skip to content

refactor(NcChip): properly document and type slots and emits #6812

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

Merged
merged 1 commit into from
May 6, 2025
Merged

refactor(NcChip): properly document and type slots and emits #6812

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 29 additions & 18 deletions src/components/NcChip/NcChip.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ export default {
'nc-chip--no-icon': !hasIcon(),
}">
<span v-if="hasIcon()" class="nc-chip__icon">
<!-- @slot The icon slot can be used to set the chip icon. Make sure that the icon is not exceeding a height of `24px`. For round icons a exact size of `24px` is recommended. -->
<slot name="icon">
<!-- The default icon wrapper uses a size of 18px to ensure the icon is not clipped by the round chip style -->
<NcIconSvgWrapper v-if="iconPath || iconSvg"
Expand All @@ -88,7 +87,6 @@ export default {
</slot>
</span>
<span class="nc-chip__text">
<!-- @slot The default slot can be used to set the text that is shown -->
<slot>{{ text }}</slot>
</span>
<NcActions v-if="canClose || hasActions()"
Expand All @@ -97,23 +95,23 @@ export default {
variant="tertiary-no-background">
<NcActionButton v-if="canClose"
close-after-click
@click="onClose">
@click="emit('close')">
<template #icon>
<NcIconSvgWrapper :path="mdiClose" :size="20" />
</template>
{{ ariaLabelClose }}
</NcActionButton>
<!-- @slot The actions slot can be used to add custom actions to the chips actions -->
<slot name="actions" />
</NcActions>
</div>
</template>

<script setup lang="ts">
import type { Slot } from 'vue'

import { mdiClose } from '@mdi/js'
import { computed, useSlots } from 'vue'
import { computed } from 'vue'
import { t } from '../../l10n.js'

import NcActions from '../NcActions/NcActions.vue'
import NcActionButton from '../NcActionButton/NcActionButton.vue'
import NcIconSvgWrapper from '../NcIconSvgWrapper/NcIconSvgWrapper.vue'
Expand Down Expand Up @@ -162,22 +160,35 @@ const props = withDefaults(defineProps<{
variant: 'secondary',
})

const emit = defineEmits(['close'])
const slots = useSlots()
const emit = defineEmits<{
/**
* Emitted when the close button is clicked
*/
close: []
}>()

const canClose = computed(() => !props.noClose)
const hasActions = () => Boolean(slots.actions?.())
const hasIcon = () => Boolean(props.iconPath || props.iconSvg || !!slots.icon?.())
const slots = defineSlots<{
/**
* The actions slot can be used to add custom actions (`NcAction*`) to the chips actions.
*/
actions: Slot
Copy link
Contributor

@ShGKme ShGKme Apr 20, 2025

Choose a reason for hiding this comment

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

Slot cannot be required

Suggested change
actions: Slot
actions?: Slot

Copy link
Contributor

@ShGKme ShGKme May 9, 2025

Choose a reason for hiding this comment

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

Also, slots?. doesn't make sense, when the type is : Slot, because Slot cannot be null/undefined.

Copy link
Contributor Author

@susnux susnux May 12, 2025

Choose a reason for hiding this comment

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

actions?: Slot

Why? Should be pretty fine according to the documentation: https://vuejs.org/api/sfc-script-setup#defineslots

Copy link
Contributor

@ShGKme ShGKme May 12, 2025

Choose a reason for hiding this comment

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

Why?

Slot is () => VNode[] which cannot be undefined

It means that !!slots.actions is the same as true

Should be pretty fine according to the documentation: https://vuejs.org/api/sfc-script-setup#defineslots

Documentation is correct only for simple cases where slots object is never used, and when this type is only used for slot names and props.

It's probably done for simplicity or a possibility in the future to add slot content typing on compile-time.

Example:

const slots = defineSlots<{
  foo(): any
}>()

// inferred as "true"
// should be "boolean"
const hasFoo = !!slots.foo

// Throws "slots.foo is not a function" because slots are never required in Vue
// Should be warned by TS
// Inferred as any
// Should be VNode[]
const fooContent = slots.foo()


/**
* Handle closing the chip (pressing the X-button)
*/
function onClose() {
/**
* Emitted when the close button is clicked
* The default slot can be used to set the text that is shown.
*/
emit('close')
}
default: Slot
Copy link
Contributor

@ShGKme ShGKme May 9, 2025

Choose a reason for hiding this comment

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

Suggested change
default: Slot
default?: Slot


/**
* The icon slot can be used to set the chip icon.
* Make sure that the icon is not exceeding a height of `--chip-size`.
* For round icons a exact size of `var(--chip-size)` is recommended.
*/
icon: Slot
Copy link
Contributor

@ShGKme ShGKme May 9, 2025

Choose a reason for hiding this comment

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

Suggested change
icon: Slot
icon?: Slot

}>()

const canClose = computed(() => !props.noClose)
const hasActions = () => Boolean(slots.actions?.())
const hasIcon = () => Boolean(props.iconPath || props.iconSvg || !!slots.icon?.())
Comment on lines +190 to +191
Copy link
Contributor

@ShGKme ShGKme May 9, 2025

Choose a reason for hiding this comment

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

The same as in previous components, we should not render slots several times, and we moved from default to icon slots in such cases to make it easy for component users to pass the slot dynamically. It is only a problem fo the default slot.

Suggested change
const hasActions = () => Boolean(slots.actions?.())
const hasIcon = () => Boolean(props.iconPath || props.iconSvg || !!slots.icon?.())
const hasActions = () => !!slots.actions
const hasIcon = () => Boolean(props.iconPath || props.iconSvg || !!slots.icon)

</script>

<style scoped lang="scss">
Expand Down
Loading