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

feat(breadcrumb): add breadcrumb component #460

Closed

Conversation

dongphuong0905
Copy link
Contributor

@dongphuong0905 dongphuong0905 commented Nov 9, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Which package are you modifying?

  • accordion
  • alert
  • alert-dialog
  • aspect-ratio
  • avatar
  • badge
  • button
  • breadcrumb
  • calendar
  • card
  • checkbox
  • collapsible
  • combobox
  • command
  • context-menu
  • data-table
  • date-picker
  • dialog
  • dropdown-menu
  • hover-card
  • icon
  • input
  • label
  • menubar
  • navigation-menu
  • pagination
  • popover
  • progress
  • radio-group
  • scroll-area
  • select
  • separator
  • sheet
  • skeleton
  • slider
  • sonner
  • spinner
  • switch
  • table
  • tabs
  • textarea
  • toast
  • toggle
  • tooltip
  • typography

What is the current behavior?

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@dongphuong0905
Copy link
Contributor Author

dongphuong0905 commented Nov 9, 2024

Close #459

@dongphuong0905 dongphuong0905 changed the title feat(breadcrumb): Add breadcrumb component feat(breadcrumb): add breadcrumb component Nov 13, 2024
Copy link
Contributor

@marcjulian marcjulian left a comment

Choose a reason for hiding this comment

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

Breadcrumb component/directive looks good. Left a few suggestions

@marcjulian
Copy link
Contributor

I noticed one more difference between HlmBreadcrumbSeparatorComponent and BreadcrumbSeparator from shadcn/ui
The list item receives additional role and aria-hidden.

https://github.com/shadcn-ui/ui/blob/805ed4120a6a8ae6f6e9714cbd776e18eeba92c7/apps/www/registry/default/ui/breadcrumb.tsx#L75-L89

CleanShot 2024-11-19 at 12 30 05

Maybe the HlmBreadcrumbSeparatorComponent renders the li with the additional properties and the markup would look like

<nav hlmBreadcrumb>
	<ol hlmBreadcrumbList>
		<li hlmBreadcrumbItem>
			<a hlmBreadcrumbLink href="/home">Home</a>
		</li>
-		<li hlmBreadcrumbItem>
-			<hlm-breadcrumb-separator />
-		</li>
+              <hlm-breadcrumb-separator />
		<li hlmBreadcrumbItem>
			<hlm-breadcrumb-ellipsis />
		</li>
	</ol>
</nav>

Alternative would be to follow the HlmBreadcrumbEllipsisComponent pattern and use a span to apply role and aria-hidden. What do you think @dongphuong0905?

@dongphuong0905
Copy link
Contributor Author

dongphuong0905 commented Nov 19, 2024

@marcjulian your suggestion is my first solution for separator component, because I want to keep it same as Shadcn. But I decided to wrap hlm-breadcrumb-separator by <li hlmBreadcrumbItem> because ol should contain li tag only. Do you think is it fine if we have span as a child of ol?
image

@marcjulian
Copy link
Contributor

marcjulian commented Nov 19, 2024

I think you are correct, its best to always have a li contained in the ol.
I have another idea to adjust HlmBreadcrumbItemDirective and we can keep the current template markup.

Apply the attributes with HlmBreadcrumbItemDirective only when a HlmBreadcrumbSeparatorComponent child is present. What do you think about this solution?

import { Directive, computed, contentChild, input } from '@angular/core';
import { hlm } from '@spartan-ng/ui-core';
import { ClassValue } from 'clsx';
import { HlmBreadcrumbSeparatorComponent } from './breadcrumb-separator.component';

@Directive({
	selector: '[hlmBreadcrumbItem]',
	standalone: true,
	host: {
		'[class]': '_computedClass()',
		// only applied if a HlmBreadcrumbSeparatorComponent is present
		'[role]': 'seperatorAttributes().role',
		'[attr.aria-hidden]': 'seperatorAttributes().ariaHidden',
	},
})
export class HlmBreadcrumbItemDirective {
	public readonly userClass = input<ClassValue>('', { alias: 'class' });

	protected readonly _computedClass = computed(() => hlm('inline-flex items-center gap-1.5', this.userClass()));

	// if a HlmBreadcrumbSeparatorComponent is present, we need to add additional attributes to the li item
	protected readonly seperator = contentChild(HlmBreadcrumbSeparatorComponent);
	protected readonly seperatorAttributes = computed(() => {
		const separator = this.seperator();
		return {
			role: separator ? 'presentation' : undefined,
			ariaHidden: separator ? 'true' : undefined,
		};
	});
}

@dongphuong0905
Copy link
Contributor Author

@marcjulian It look nice. I think this is the only way to keep the template markup style and result after render same as Shadcn.
I will update my code

@dongphuong0905
Copy link
Contributor Author

dongphuong0905 commented Nov 19, 2024

@marcjulian an idea popped into my head as i updated my code.

I will change the selector of HlmBreadcrumbSeparator like this:

import { Component, computed, input } from '@angular/core';
import { lucideChevronRight } from '@ng-icons/lucide';
import { hlm } from '@spartan-ng/ui-core';
import { HlmIconComponent, provideIcons } from '@spartan-ng/ui-icon-helm';
import { ClassValue } from 'clsx';

@Component({
        // Change the selector 
	selector: '[hlmBreadcrumbSeparator]',
	standalone: true,
	imports: [HlmIconComponent],
	providers: [provideIcons({ lucideChevronRight })],
	host: {
		role: 'presentation',
		'[class]': '_computedClass()',
		'[attr.aria-hidden]': 'true',
	},
	template: `
      	<ng-content>	
			<hlm-icon name="lucideChevronRight" class="h-3.5 w-3.5" />
		</ng-content>
  	`,
})
export class HlmBreadcrumbSeparatorComponent {
	public readonly userClass = input<ClassValue>('', { alias: 'class' });

	protected readonly _computedClass = computed(() => hlm('h-3.5', this.userClass()));
}

And the template markup will be like:

<li hlmBreadcrumbSeparator></li>

For case custom separator:

<li hlmBreadcrumbSeparator>
    <hlm-icon name="lucideSlash" class="h-4 w-4" />
</li>

With this approach, we can keep the HlmBreadcrumbItem simple and still have the same result 😄
image

How do you think about this approach?

@marcjulian
Copy link
Contributor

Thats an even better solution. Just a question is h-3.5 needed on the li element?

Perhaps protected readonly _computedClass = computed(() => hlm(this.userClass())); is enough for HlmBreadcrumbSeparatorComponent

@dongphuong0905
Copy link
Contributor Author

dongphuong0905 commented Nov 20, 2024

@marcjulian
I set h-3.5 because default of hlm-icon is inline-flex, so the separator won't be vertical centered
image

But now I found a solution to fix it, I will set the hlm-icon is flex, and also add [&>hlm-icon]:flex [&>hlm-icon]:w-3.5 [&>hlm-icon]:h-3.5 instead of h-3.5 to make separator same as Shadcn. ( In Shadcn, they use [&>svg]:w-3.5 [&>svg]:h-3.5 for separator)
Now the separator will like this:

import { Component, computed, input } from '@angular/core';
import { lucideChevronRight } from '@ng-icons/lucide';
import { hlm } from '@spartan-ng/ui-core';
import { HlmIconComponent, provideIcons } from '@spartan-ng/ui-icon-helm';
import { ClassValue } from 'clsx';

@Component({
	selector: '[hlmBreadcrumbSeparator]',
	standalone: true,
	imports: [HlmIconComponent],
	providers: [provideIcons({ lucideChevronRight })],
	host: {
		role: 'presentation',
		'[class]': '_computedClass()',
		'[attr.aria-hidden]': 'true',
	},
	template: `
      	        <ng-content>	
			<hlm-icon name="lucideChevronRight" />
		</ng-content>
  	`,
})
export class HlmBreadcrumbSeparatorComponent {
	public readonly userClass = input<ClassValue>('', { alias: 'class' });

	protected readonly _computedClass = computed(() =>
		hlm('[&>hlm-icon]:w-3.5 [&>hlm-icon]:h-3.5 [&>hlm-icon]:flex', this.userClass()),
	);
}

And DOM will like this:
image

Result:
image
image

@marcjulian
Copy link
Contributor

That looks good too me!

@marcjulian
Copy link
Contributor

marcjulian commented Nov 20, 2024

The breadcrumb component looks clean! @goetzrobin this looks good and uses a similar approach as the pagination component. This contains a breaking change because of the updated ng-icon package, as stated in the PR description.

@dongphuong0905
Copy link
Contributor Author

dongphuong0905 commented Nov 21, 2024

@marcjulian @goetzrobin I saw @ng-icon was updated to v 29.10.0 in latest main, so this PR will not contain any breaking change.

@goetzrobin
Copy link
Collaborator

@dongphuong0905 there's a couple issues with linting that need to be addressed and for the commit message it just needs to start with lowercase. Let me know if you need help with this!

@dongphuong0905
Copy link
Contributor Author

@goetzrobin I fixed the lint error, but for the commitlint, I have a uppercase on the first message. Do you have any idea to fix this commit message?

@goetzrobin
Copy link
Collaborator

I can take care of it while merging!
But just in case you are curious you can use the following command: git rebase -i HEAD~3 where 3 is the number of commits you want to go back and then change the message in Vim

@dongphuong0905
Copy link
Contributor Author

@goetzrobin Thanks a lot 😄

@dongphuong0905 dongphuong0905 mentioned this pull request Nov 23, 2024
2 tasks
@MerlinMoos
Copy link
Contributor

MerlinMoos commented Nov 23, 2024

Hey @dongphuong0905 I checked the code, I have one recommendation. For the seperator you are always providing the chevronIconRight. Maybe it make sense to create a brain directive which is handling that dynamicly like

import {
	AfterContentInit,
	contentChildren,
	Directive,
	ElementRef,
	inject,
	Injector,
	ViewContainerRef,
} from '@angular/core';
import { provideIcons } from '@ng-icons/core';
import { lucideChevronRight } from '@ng-icons/lucide';
import { HlmIconComponent } from '@spartan-ng/ui-icon-helm';

@Directive({
	selector: '[brnBreadcrumbSeparator]',
	standalone: true,
	host: {
		role: 'presentation',
		'[attr.aria-hidden]': 'true',
	},
})
export class BrnBreadcrumbSeparatorDirective implements AfterContentInit {
	private readonly _viewContainerRef = inject(ViewContainerRef);
	private readonly _elementRef = inject(ElementRef);
	protected _icon = contentChildren(HlmIconComponent);

	ngAfterContentInit(): void {
		if (this._icon().length === 0) {
			const injector = Injector.create({
				providers: [provideIcons({ lucideChevronRight })],
			});
			const componentRef = this._viewContainerRef.createComponent(HlmIconComponent, {
				injector,
			});
			componentRef.setInput('name', 'lucideChevronRight');

			const dynamicElement = componentRef.location.nativeElement;
			this._elementRef.nativeElement.appendChild(dynamicElement);
		}
	}
}

After that angular don't have to provide every time the icon and the hlm-icon in case that someone want to have custom seperator

@dongphuong0905
Copy link
Contributor Author

Hi @MerlinMoos,
In separator, I used the lucideChevronRight as a fallback of <ng-content>. It means if we add a custom icon for separator, Angular won't render the lucideChevronRight.
This is the new feature of Angular v18 https://blog.angular.dev/angular-v18-is-now-available-e79d5ac0affe#2a2b

IMHO, I think code will be more simple and maintainable with this approach.

This is the screenshot of DOM when I use custom separator, It only have slash icon 😄
image

@dongphuong0905
Copy link
Contributor Author

Hi @goetzrobin,
I created a new PR to replace this one to fix the commit lint: #499
You can merge the new PR and close this one. I think It can save your time 😄

@dongphuong0905
Copy link
Contributor Author

PR #499 was merged. So I will close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants