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

enh: added option to render icon and text for breadcrumb #5216

Merged

Conversation

emoral435
Copy link
Contributor

@emoral435 emoral435 commented Feb 6, 2024

For nextcloud/server#43604

🖼️ Screenshots

🏡 After

firefox_FroddX5muA

Summary

  • To prevent a breaking change, I decided to add a prop that user configures to enable the nationality to add text to accompany a icon breadcrumb that is a NcButton, and not an NcAction.
  • Since default for iconText is false, all previous library users of NcBreadcrumb will not have breaking changes
  • Library now allows user to have NcButton with just text, just icon, and now icon and text

🏁 Checklist

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

@emoral435 emoral435 force-pushed the enh/personal-files/breadcrumbs/added-extra-NcButton-default branch from 6788751 to 3fdda27 Compare February 15, 2024 15:07
@emoral435 emoral435 self-assigned this Feb 15, 2024
@emoral435 emoral435 added enhancement New feature or request 3. to review Waiting for reviews design Design, UX, interface and interaction design labels Feb 15, 2024
@emoral435 emoral435 added this to the 8.6.3 milestone Feb 15, 2024
@emoral435 emoral435 marked this pull request as ready for review February 15, 2024 15:12
Pytal
Pytal previously requested changes Feb 16, 2024
src/components/NcBreadcrumb/NcBreadcrumb.vue Outdated Show resolved Hide resolved
@emoral435 emoral435 force-pushed the enh/personal-files/breadcrumbs/added-extra-NcButton-default branch 2 times, most recently from 5d34577 to f5a4076 Compare February 16, 2024 14:25
@emoral435 emoral435 requested a review from Pytal February 16, 2024 14:26
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.

These changes have a logic flaw and lead to the name being always shown.

src/components/NcBreadcrumb/NcBreadcrumb.vue Outdated Show resolved Hide resolved
src/components/NcBreadcrumb/NcBreadcrumb.vue Outdated Show resolved Hide resolved
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
@emoral435 emoral435 force-pushed the enh/personal-files/breadcrumbs/added-extra-NcButton-default branch from f5a4076 to 942ef16 Compare February 16, 2024 18:38
@emoral435
Copy link
Contributor Author

emoral435 commented Feb 16, 2024

@raimund-schluessler I agree with your requested changes. In order to simplify the logic, whilst not making any breaking changes, here are the logic design choices I made:

  • First check if icon or iconSlot is passed, and there is no default slot being passed in
    • If so, load icon, then check to see if we want to load accompanying text
  • If neither icon nor default slot is passed, then we need to have a fail safe. This fail safe is loading in the name text
  • If default slot IS given, then the same logic as previous commits is used

Does this align with your requested changes?

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Feb 16, 2024

Does this align with your requested changes?

Your changes lead to the desired outcome, but they are quite a bit more complicated then they need to be (especially the duplication of NcButton). I pushed a commit to show what i meant.

@raimund-schluessler raimund-schluessler force-pushed the enh/personal-files/breadcrumbs/added-extra-NcButton-default branch from e27c9d5 to 27b6bf5 Compare February 16, 2024 21:59
@emoral435
Copy link
Contributor Author

emoral435 commented Feb 16, 2024

bit more complicated then they need to be

Ah, I see what you mean. I saw this problem while developing and I like this solution, but I have a question.

<NcButton v-if="(name || icon || $slots.icon) && !$slots.default"

name prop is required. So unless users explicitly put "", the check will never check if the icon prop or icon slot is inputted. From what I gathered, the checks in place where to always at least have some information shown in the button.
This new change means that if no default slot was input, but also no icon prop or icon slot, then the button will just be empty, no?

@raimund-schluessler
Copy link
Contributor

Without icon, icon slot and default slot, we still show the name. But yes, if that is also empty, nothing will be shown, because nothing was provided. I don't think that is an issue. NcButton works the same.

@susnux susnux removed this from the 8.6.3 milestone Feb 18, 2024
@susnux susnux added this to the 8.7.0 milestone Feb 18, 2024
@skjnldsv skjnldsv merged commit c510ad6 into master Feb 19, 2024
18 checks passed
@skjnldsv skjnldsv deleted the enh/personal-files/breadcrumbs/added-extra-NcButton-default branch February 19, 2024 08:29
@raimund-schluessler
Copy link
Contributor

/backport to next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants