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

Feature/add breadcrumb to header #29

Merged
merged 5 commits into from
Aug 27, 2024
Merged

Conversation

doriengr
Copy link
Contributor

Closes #24
Closes #23

@doriengr doriengr self-assigned this Aug 23, 2024
@doriengr doriengr added the need-reviewer Send notification in Discord label Aug 23, 2024
Comment on lines 5 to 15
const pathNameMap: { [key: string]: string } = {
'/': 'Dashboard',
'/sensors': 'Sensoren',
'/settings': 'Einstellungen',
'/team': 'Mitarbeitende',
'/treecluster': 'Baumgruppen',
'/vehicles': 'Fahrzeuge',
'/waypoints/': 'Einsatzpläne',
'/waypoints/new': 'Neuer Einsatzplan',
'/map/': 'Kataster',
};
Copy link
Member

Choose a reason for hiding this comment

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

What if we have the route /treecluster/:id. Then we need to update the logic here.

My suggestion, maybe we should move the hole logic from this component to it's own useBreadcrumb hook, witch converses the logic and returns the title based on the route.

Comment on lines 18 to 26
const breadcrumbs = matches
.filter(({ pathname }) => pathname !== '/')
.map(({ pathname }) => {
const title = pathNameMap[pathname] || 'Kein Titel vorhanden';
return {
title,
path: pathname,
};
});
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should wrap this in a useMemo so it doesn't have to be recalculated on every rerender. It's only dependent on matches.

Suggested change
const breadcrumbs = matches
.filter(({ pathname }) => pathname !== '/')
.map(({ pathname }) => {
const title = pathNameMap[pathname] || 'Kein Titel vorhanden';
return {
title,
path: pathname,
};
});
const breadcrumbs = useMemo(() => matches
.filter(({ pathname }) => pathname !== '/')
.map(({ pathname }) => {
const title = pathNameMap[pathname] || 'Kein Titel vorhanden';
return {
title,
path: pathname,
};
}), [matches]);

Comment on lines +44 to +54
{breadcrumbs.map((breadcrumb, key) => (
<li key={key} className="flex items-center">
<ChevronRight className="w-3.5 stroke-1" />
<Link
className="data-[status=active]:text-dark data-[status=active]:font-semibold transition-all ease-in-out duration-300 hover:text-green-dark hover:data-[status=active]:text-green-dark"
to={breadcrumb.path}
>
{breadcrumb.title}
</Link>
</li>
))}
Copy link
Member

@choffmann choffmann Aug 23, 2024

Choose a reason for hiding this comment

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

Is there a case where the list gets so big that it no longer fits? If so, we should introduce a limit or something, maybe shadncn's solution with the dots makes sense.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use the component first, but I noticed that the breadcrumb uses real and does not go through TanStack's router. Is this intentional, so that the page reloads completely?

Comment on lines 68 to 82
<li key={key}>
<Link
to={card.url}
aria-label={card.linkLabel}
className={`shadow-card p-6 rounded-xl group flex flex-col gap-4 transition-all ease-in-out duration-300 border
${key % 2 !== 0 ? 'border-green-dark bg-green-dark-50 hover:bg-green-dark-100' : 'border-green-light bg-green-light-50 hover:bg-green-light-100'} `}
>
<h3 className="font-lato text-lg text-dark font-semibold">{card.headline}</h3>
<p>{card.description}</p>
<p className="font-lato font-semibold text-green-dark flex items-center gap-x-2">
<span>{card.linkLabel}</span>
<MoveRight className="transition-all ease-in-out duration-300 group-hover:translate-x-2"/>
</p>
</Link>
</li>
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this could be a component of its own?

Copy link
Member

@choffmann choffmann left a comment

Choose a reason for hiding this comment

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

👍

@choffmann choffmann self-requested a review August 25, 2024 19:31
@doriengr doriengr merged commit 59821c3 into develop Aug 27, 2024
1 check passed
@doriengr doriengr deleted the feature/add-breadcrumb-to-header branch August 27, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-reviewer Send notification in Discord
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breadcrumb Dashboard with quick links
2 participants