refactor: Migrate `NcButton` from render function to template #6033

susnux · 2024-08-30T18:06:24Z

☑️ Resolves

Make it a template component instead of a render function
Add a text prop to allow passing text content as prop
Add noIconAriaHidden to allow making the icon appear on the accessible tree

🏁 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

src/components/NcButton/NcButton.vue

ShGKme · 2025-01-19T22:55:40Z

src/components/NcButton/NcButton.vue

+	/**
+	 * By default the icon is set to `aria-hidden="true"` to hide it from the accessibility tree.
+	 * But sometimes this is not desired, e.g. for a loading icon with an accessible name,
+	 * setting this prop will make the icon appear in the accessible tree.
+	 * @default false
+	 * @since 9.0.0
+	 */
+	noIconAriaHidden: {
+		type: Boolean,
+		default: false,
+	},


Is there any other cases, except loading? If it's only for loading, I think, it's more useful to provide built-in loading feature.

You could also have other accessible names on icons if you use the icon only variant.
But yes in that case it probably makes sense to set the aria label directly on the button.

My fear with non-hidden icons with accessible names is that developers will use icon name as an accessible name (like it's done in some icon libraries) while it's usually not correct:

Icon names are not translated

Icon name may not represent the action correctly, like alarm name for set reminder or laptop for edit locally

Maybe this not needed at all? Imagine its a loading spinner for a dialog button to save a form:

What about this:
<NcButton aria-label="Saving…" title="Saving…"><template #icon><NcLoadingIcon /></template></NcButton>

Meaning put it directly into the aria-label.
So we need no prop at all here.

1. Make it a template component instead of a render function 2. Add a `text` prop to allow passing text content as prop 3. Add `noIconAriaHidden` to allow making the icon appear on the accessible tree Co-authored-by: Ferdinand Thiessen <[email protected]> Co-authored-by: Grigorii K. Shartsev <[email protected]> Signed-off-by: Ferdinand Thiessen <[email protected]>

ShGKme · 2025-01-29T20:52:35Z

src/components/NcButton/types.ts

+	 * This can be overwritten by using the *default* slot.
+	 * @since 9.0.0
+	 */
+	text?: string
+
+	/**
+	 * Specifies the button variant.
+	 * If left empty, the default button style will be applied.
+	 *
+	 * @default 'secondary'
+	 */
+	variant?: EnumOrValue<ButtonVariant>
+
+	/**
+	 * Specifies the button native type
+	 * If left empty, the default "button" type will be used.
+	 *
+	 * @default 'button'
+	 */
+	type?: EnumOrValue<ButtonType>
+
+	/**
+	 * Specifies whether the button should span all the available width.
+	 * By default, buttons span the whole width of the container.
+	 * @default false
+	 */
+	wide?: boolean
+
+	/**
+	 * Always try to provide an aria-label to your button.
+	 *
+	 * Make it more specific than the button's name by provide some more context.
+	 * E.g. if the name of the button is "send" in the Mail app,
+	 * the aria label could be "Send email".
+	 */
+	ariaLabel?: string
+
+	/**
+	 * Providing the href attribute turns the button component into an `a` element.
+	 */
+	href?: string
+
+	/**
+	 * Target for the `a` element if `href` is set.
+	 * @default '_self'
+	 */
+	target?: string
+
+	/**
+	 * When `href` is set this will make the browser try to download the target.
+	 * If a string is passed the browser will use it as the filename.
+	 *
+	 * Note that the browser might adjust it for allowed characters (e.g. '/' or '\').
+	 * Also this only works with same-origin URLs and `blob:` or `data:` schemas.
+	 * Moreover a `Content-Disposition` header set by the server will override the filename.
+	 */
+	download?: string|true
+
+	/**
+	 * The pressed state of the button if it has a checked state.
+	 * This will add the `aria-pressed` attribute and for the button to have the primary style in checked state.
+	 *
+	 * Note: Pressed state is not supported for links.
+	 */
+	pressed?: boolean
+
+	/**
+	 * Providing the to attribute turns the button component into a `router-link` element.
+	 *
+	 * Note: This takes precedence over the href attribute.
+	 */
+	to?: string | RouteLocation
+}
+
+export interface NcButtonEvents {
+	/**
+	 * Emitted when the button was clicked.
+	 */
+	click: [e: MouseEvent]
+
+	/**
+	 * If the button is a toggle button (`pressed` state is boolean),
+	 * then this will be emitted if the user toggled the state.
+	 */
+	'update:pressed': [pressed: boolean]
+}
+
+export interface NcButtonSlots {
+	/**
+	 * Custom button content.
+	 * This can be used for custom formatted content, ensure to not include any interactive elements.
+	 * For plain text it is preferred to use the `text` prop instead.
+	 */
+	default?: Slot
+
+	/**
+	 * Icon (optional) to show within the button
+	 */
+	icon?: Slot
+}


Please let's keep component props/events/slots in the component.

SFC concept is about having everything that is direct component definition in one file. Props are very part of the component, even more important than styles. When a component is developed/debugged, a developer very often needs to see component props and other interfaces.

ShGKme · 2025-01-29T20:53:06Z

src/utils/enumOrValue.ts

+/**
+ * Helper to allow either the enum T or its string variant.
+ *


Or just remove enum-s as a kind of legacy feature?

ShGKme · 2025-01-29T20:54:39Z

src/components/NcButton/NcButton.vue

+		:class="[
+			'button-vue',


To separate static values and dynamic binding

Suggested change

:class="[

'button-vue',

class="button-vue"

:class="[

ShGKme · 2025-01-29T20:55:08Z

src/components/NcButton/NcButton.vue

-			default: false,
-		},
+		]"
+		:disabled="disabled"


Vue 3.5 💅✨

Suggested change

:disabled="disabled"

:disabled

ShGKme · 2025-01-29T20:55:55Z

src/components/NcButton/NcButton.vue

+import isSlotPopulated from '../../utils/isSlotPopulated'
+import logger from '../../utils/logger'


Suggested change

import isSlotPopulated from '../../utils/isSlotPopulated'

import logger from '../../utils/logger'

import isSlotPopulated from '../../utils/isSlotPopulated.ts'

import logger from '../../utils/logger.ts'

ShGKme · 2025-01-29T20:58:08Z

src/components/NcButton/NcButton.vue

 		/**
-		 * The pressed state of the button if it has a checked state
-		 * This will add the `aria-pressed` attribute and for the button to have the primary style in checked state.
+		 * Update the current pressed state of the button (if the `pressed` property was configured)


Let's keep docs only in interface docs. Then it's compatible with all the tooling

ShGKme · 2025-01-29T20:59:10Z

src/components/NcButton/NcButton.vue

 		cursor: default;
-		& * {
+		:deep(*) {


Does nothing in Vue 3

Suggested change

:deep(*) {

* {

ShGKme · 2025-01-29T21:01:18Z

src/components/NcButton/NcButton.vue

+			// Warning but we made sure that the log level is DEBUG above so we do not spam users
+			logger.warn('You need to fill either the text or the ariaLabel props in the button component.')


This works if a developer dynamically removes aria-label (never happens), but it doesn't work if a developer removes text on mobile (happens often).

And I'm still not fun of rendering content 3 times instead of 1...

In watch

In v-if

In content render

2 + 3 happens each re-render

I'd consider getting rid of this check at all in flavor of check on e2e test level.

Developers still successfully ignore this warning :(

Much effort for only one of many places where we need to worry about a11y

susnux mentioned this pull request Aug 30, 2024

[Discussion] Add JSX support to simplify render function components and Vue 2 + Vue 3 support #6024

Open

susnux force-pushed the feat/refactor-nc-button branch from e8fd644 to 1b5c7b5 Compare August 30, 2024 19:03

susnux mentioned this pull request Jan 18, 2025

chore: Prepare release of v9.0.0-alpha.6 #6397

Merged

15 tasks

susnux force-pushed the feat/refactor-nc-button branch 2 times, most recently from ab9aed0 to 04e0282 Compare January 19, 2025 16:08

susnux added 3. to review Waiting for reviews feature: button vue 3 Related to the vue 3 migration feature: functions composables, functions, mixins and other non-components refactor ♻️ Pull request that is neither a fix nor a feature labels Jan 19, 2025

susnux modified the milestones: 9.0.0-alpha.6, 8.23.0, 9.0.0 Jan 19, 2025

susnux changed the title ~~feat: Refactor NcButton~~ refactor: Migrate NcButton from render function to template Jan 19, 2025

susnux removed the feature: functions composables, functions, mixins and other non-components label Jan 19, 2025

susnux force-pushed the feat/refactor-nc-button branch from 04e0282 to c0dd0de Compare January 19, 2025 18:10

susnux marked this pull request as ready for review January 19, 2025 18:14

susnux requested review from raimund-schluessler and ShGKme January 20, 2025 10:38

ShGKme reviewed Jan 21, 2025

View reviewed changes

susnux force-pushed the feat/refactor-nc-button branch from 541f542 to 228fb39 Compare January 22, 2025 11:12

susnux and others added 2 commits January 29, 2025 10:51

fixup! feat: Refactor NcButton

562a8b7

susnux force-pushed the feat/refactor-nc-button branch from 228fb39 to 562a8b7 Compare January 29, 2025 11:20

ShGKme reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Migrate `NcButton` from render function to template #6033

refactor: Migrate `NcButton` from render function to template #6033

susnux commented Aug 30, 2024 •

edited

Loading

ShGKme Jan 19, 2025

susnux Jan 21, 2025

ShGKme Jan 22, 2025

susnux Jan 22, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

ShGKme Jan 29, 2025

		import isSlotPopulated from '../../utils/isSlotPopulated'
		import logger from '../../utils/logger'

		// Warning but we made sure that the log level is DEBUG above so we do not spam users
		logger.warn('You need to fill either the text or the ariaLabel props in the button component.')

refactor: Migrate NcButton from render function to template #6033

Are you sure you want to change the base?

refactor: Migrate NcButton from render function to template #6033

Conversation

susnux commented Aug 30, 2024 • edited Loading

☑️ Resolves

🏁 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: Migrate `NcButton` from render function to template #6033

refactor: Migrate `NcButton` from render function to template #6033

susnux commented Aug 30, 2024 •

edited

Loading