Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
190 changes: 190 additions & 0 deletions src/components/NcFormBox/NcFormBoxItem.vue
Copy link
Contributor

Choose a reason for hiding this comment

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

new folder structure? Why not put it inside components/NcFormBox/ like we do with some other shared components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it clear that it is not a public API and prevent "I forgot to exclude from the docs" situations.

But it is an internal thing, so we can refactor the structure any moment later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything Public API/design/implementation related?

Since this is the only one comment in the review, I don't know, if it is the full review or you submitted with this comment only

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything Public API/design/implementation related?

No otherwise its fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

To make it clear that it is not a public API

But only what is exported from the index.ts is public API.
IMHO having a new folder structure is more confusing than helpful.

and prevent "I forgot to exclude from the docs" situations.

True but thats due to non-explicitly defined styleguidist config but also not hard to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I would prefer moving it in this PR back to the normal location /src/components/NcFormBox/NcFormBoxItem.vue and do a follow up where we e.g. move all internal components and composables to consistent new locations. So we can just continue this one without mixing build related changes.

Copy link
Contributor Author

@ShGKme ShGKme Nov 5, 2025

Choose a reason for hiding this comment

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

components/NAME/NAME.vue

Even if we call components/NAME/NAME.vue public, it doesn't specify the position for the private component, does it? Works if it is a subcomponent of a specific public one, but what if it is shared?

  • components/Foo/Foo.vue -> uses -> Shared.vue
  • components/Bar/Bar.vue -> uses -> Shared.vue

Ok, Foo.vue and Bar.vue are public. Where Shared.vue is supposed to be?
If in Foo - why not Bar. If in Bar - why not Foo.

every other vue file in that folder is private

The following components are "every other" and they are public:

  • components/NcRichContenteditable/NcMentionBubble.vue
  • components/NcRichContenteditable/NcAutoCompleteResult.vue

So:

  1. No, not only such paths are public
  2. No, having such paths doesn't specify the path for private even if p.1 were actual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also its not really matter as a developer if the component is public or not, because you can use both.

It does matter a lot. In a private component I can do everything, rewrite it, rename, change the interface, throw it away. In a public component I need to be careful and not break anything. It is easy to change it and hard to bring back after being released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a completely different folder

I also proposed 2 alternative names for the folder and 1 alternative location.

Again, I'm fine with a different location, folder name or both.

I'm not fine with just mixing public with private files together and only separate by including/excluding in re-export and the list on the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@susnux I pushed a commit moving the component to the components/NcFormBox/NcFormBoxItem.vue. I don't think this is the best place for it, but it can be discussed later, outside the NcFormBoxButton PR.

Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
<!--
- SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
- SPDX-License-Identifier: AGPL-3.0-or-later
-->

<script setup lang="ts">
import type { Slot } from 'vue'
import type { VueClassType } from '../../utils/VueTypes.ts'

import { useNcFormBox } from '../../components/NcFormBox/useNcFormBox.ts'
import { createElementId } from '../../utils/createElementId.ts'
import { isLegacy } from '../../utils/legacy.ts'

defineOptions({ inheritAttrs: false })

const {
tag,
label = undefined,
description = undefined,
invertedAccent = false,
class: rootClasses = undefined,
itemClasses = undefined,
} = defineProps<{
/** Interactive item element's tag */
tag: string
/** Main Label */
label?: string
/** Optional description below the label, also used for the aria-describedby */
description?: string
/** Accent on the description instead of the label */
invertedAccent?: boolean
/** Root element classes */
class?: VueClassType
/** Interactive item classes */
itemClasses?: VueClassType
}>()

defineEmits<{
/** Click on the item */
click: [event: MouseEvent]
}>()

const slots = defineSlots<{
/** Item's label custom content */
default?: Slot<{
/** IDRef of the description element if present */
descriptionId?: string
}>
/** Custom description content */
description?: Slot<{
/** IDRef of the description element if present */
descriptionId?: string
}>
/** Icon content */
icon?: Slot
}>()

const { formBoxItemClass } = useNcFormBox()

const descriptionId = createElementId()
const hasDescription = () => !!description || !!slots.description
</script>

<template>
<div
:class="[
rootClasses,
$style.formBoxItem,
formBoxItemClass,
{
[$style.formBoxItem_inverted]: invertedAccent && hasDescription(),
[$style.formBoxItem_legacy]: isLegacy,
},
]">
<span :class="$style.formBoxItem__content">
<component
:is="tag"
:class="[$style.formBoxItem__element, itemClasses]"
v-bind="$attrs"
@click="$emit('click', $event)">
<slot :description-id>
{{ label || '⚠️ Label is missing' }}
</slot>
</component>
<span v-if="hasDescription()" :id="descriptionId" :class="$style.formBoxItem__description">
<slot name="description">
{{ description }}
</slot>
</span>
</span>
<span :class="$style.formBoxItem__icon">
<slot name="icon" :description-id>
⚠️ Icon is missing
</slot>
</span>
</div>
</template>

<style lang="scss" module>
.formBoxItem {
--nc-form-box-item-border-width: 1px;
--nc-form-box-item-min-height: 40px; // Special size defined by the design
--form-element-label-offset: calc(var(--border-radius-element) + var(--default-grid-baseline));
--form-element-label-padding: calc(var(--form-element-label-offset) - var(--nc-form-box-item-border-width));
// New colors we don't yet have in theming
// TODO: add new colors to the theming
--color-primary-element-extra-light: hsl(from var(--color-primary-element-light) h s calc(l * 1.045));
--color-primary-element-extra-light-hover: hsl(from var(--color-primary-element-light-hover) h s calc(l * 1.045));
position: relative;
display: flex;
align-items: center;
gap: calc(2 * var(--default-grid-baseline));
min-height: var(--nc-form-box-item-min-height);
padding-inline: var(--form-element-label-padding);
border: 1px solid var(--color-primary-element-extra-light-hover);
border-bottom-width: 2px;
border-radius: var(--border-radius-element);
background-color: var(--color-primary-element-extra-light);
color: var(--color-primary-element-light-text);
transition-property: color, border-color, background-color;
transition-duration: var(--animation-quick);
transition-timing-function: linear;
user-select: none;
cursor: pointer;

* {
cursor: inherit;
}

&:has(:disabled) {
cursor: default;
opacity: 0.5;
}

&:hover:not(:has(:disabled)) {
color: var(--color-primary-element-light-text);
background-color: var(--color-primary-element-extra-light-hover);
}

&:has(:focus-visible) {
outline: 2px solid var(--color-main-text);
box-shadow: 0 0 0 4px var(--color-main-background);
}

&.formBoxItem_legacy {
--nc-form-box-item-border-width: 0px;
border: none;
}

&.formBoxItem_inverted {
.formBoxItem__element {
color: var(--color-text-maxcontrast);
}

.formBoxItem__description {
color: inherit;
}
}
}

.formBoxItem__content {
flex: 1;
display: flex;
flex-direction: column;
padding-block: calc(2 * var(--default-grid-baseline));
overflow-wrap: anywhere;
}

// A trick for accessibility:
// make entire component clickable while internally splitting the interactive item and the description
.formBoxItem__element::after {
content: '';
position: absolute;
inset: 0;
}

.formBoxItem__description {
color: var(--color-text-maxcontrast);
}

.formBoxItem__icon {
display: flex;
align-items: center;
justify-content: flex-end;
}
</style>

<docs>
An internal component
</docs>
Loading
Loading