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.

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

Jump to bottom

enh(breadcrumb): conditionally renders button when no redirection link given #5077

Merged
merged 2 commits into from
Jan 17, 2024
Merged

enh(breadcrumb): conditionally renders button when no redirection link given #5077

merged 2 commits into from
Jan 17, 2024

Conversation

emoral435
Copy link
Contributor

☑️ For nextcloud/server#42624

This fixes the semantic problems when no redirection link is provided to the NcBreadcrumb component, so it renders our own NcButton. To fix the problem references, nextcloud/server#42624, we would have to update NcDialog's library version for vue

🖼️ Screenshots

🏚️ Before 🏡 After
image firefox_SmRJdHmGO3

🏁 Checklist

  • ⛑️ Tests are included or are not applicable - Not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@emoral435 emoral435 added enhancement New feature or request 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 16, 2024
@emoral435 emoral435 added this to the 8.5.0 milestone Jan 16, 2024
@emoral435 emoral435 self-assigned this Jan 16, 2024
@emoral435
Copy link
Contributor Author

emoral435 commented Jan 16, 2024 •
edited
Loading

after reviews given on this PR: nextcloud-libraries/nextcloud-dialogs#1169 , came to the conclusion that rendering a button for the breadcrumbs makes the most sense semantically 👍

@emoral435 emoral435 force-pushed the feat/breadcrumbs/42624/a11y/allow-breadcrumbs-to-render-as-buttons branch from 22149cd to 000d3a7 Compare January 16, 2024 17:48
@raimund-schluessler
Copy link
Contributor

Since NcButton can be a router-link, an a or just a button, I would propose to leave the decision what to render to NcButton completely. This saves us some code in NcBreadcrumb, also for the CSS.

@emoral435 emoral435 force-pushed the feat/breadcrumbs/42624/a11y/allow-breadcrumbs-to-render-as-buttons branch from 000d3a7 to 2c7aab6 Compare January 16, 2024 20:27
@emoral435
Copy link
Contributor Author

Changed to use NcButton 👍!

Pytal
Pytal reviewed Jan 16, 2024
View reviewed changes
src/components/NcBreadcrumb/NcBreadcrumb.vue
:title="title"
:aria-label="icon ? name : undefined"
v-bind="linkAttributes"
v-on="$listeners">
<!-- @slot Slot for passing a material design icon. Precedes the icon and name prop. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be kept for the docs 😉

raimund-schluessler
raimund-schluessler requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

The CSS changes break the hiding of the breadcrumbs. @emoral435 Do you mind if I push a commit to fix this?

@raimund-schluessler
Copy link
Contributor

The CSS changes break the hiding of the breadcrumbs. @emoral435 Do you mind if I push a commit to fix this?

@emoral435 I hope you don't mind that I pushed a commit here to handle the visual changes introduced by switching to NcButton. The component is relatively complex and required to interact with NcBreadcrumbs to make hiding breadcrumbs work. So I figured that going back and forth with reviews and adjustments would take quite some time and adjusting it directly would be faster. Sorry if that messed with you getting to know the code here 🙈 And welcome to nextcloud/vue 🙂

@raimund-schluessler raimund-schluessler force-pushed the feat/breadcrumbs/42624/a11y/allow-breadcrumbs-to-render-as-buttons branch from b4bb21f to 7347ea6 Compare January 16, 2024 21:48
@emoral435 emoral435 force-pushed the feat/breadcrumbs/42624/a11y/allow-breadcrumbs-to-render-as-buttons branch from 7347ea6 to 3062161 Compare January 17, 2024 01:57
@emoral435
Copy link
Contributor Author

I personally did not mind! Thank you for the changes :) enabling auto-merge 🫡

@emoral435 emoral435 enabled auto-merge January 17, 2024 01:57
@emoral435 emoral435 merged commit 9571889 into master Jan 17, 2024
15 checks passed
@emoral435 emoral435 deleted the feat/breadcrumbs/42624/a11y/allow-breadcrumbs-to-render-as-buttons branch January 17, 2024 01:58
@raimund-schluessler raimund-schluessler mentioned this pull request Jan 18, 2024
Closed
@Pytal Pytal mentioned this pull request Jan 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raimund-schluessler raimund-schluessler raimund-schluessler approved these changes

@Pytal Pytal Pytal approved these changes

@susnux susnux Awaiting requested review from susnux

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees

@emoral435 emoral435

Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities enhancement New feature or request
Projects
None yet
Milestone
8.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@emoral435 @raimund-schluessler @Pytal