-
Notifications
You must be signed in to change notification settings - Fork 49
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(vue-demo): add dynamic breadcrumbs #1369
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@shopware/api-client
@shopware/api-gen
@shopware-pwa/composables-next
@shopware-pwa/cms-base
@shopware-pwa/helpers-next
@shopware-pwa/nuxt3-module
commit: |
@kstala could you test this please 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feature incoming! We need to clarify a few things here first :)
|
||
useBreadcrumbs(breadcrumbs); | ||
const { buildDynamicBreadcrumbs } = useBreadcrumbs(); | ||
buildDynamicBreadcrumbs(props.navigationId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's happening asynchronously, so I have two questions:
- how are you assuring it's loaded on Server Side Render?
- isn't there a blink on client side? What about showing previous breadcrumbs and replace them when new ones are loaded? Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, changed
@@ -48,8 +66,38 @@ export function useBreadcrumbs( | |||
_breadcrumbs.value = []; | |||
}; | |||
|
|||
const getCategoryBreadcrumbs = async (categoryId: string) => { | |||
const response = await apiClient.invoke( | |||
"readBreadcrumb get /breadcrumb/{id}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this endpoint is not only to load category breadcrumbs, I'm not sure about exposing it here, what is the designed flow here looks like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This endpoint is only for the category breadcrumbs. For the product entity we will also receive, category breadcrumbs
return response.data; | ||
}; | ||
|
||
const buildDynamicBreadcrumbs = async (categoryId: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about product breadcrumbs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is the same method, for product we are also getting category breadcrumbs
} catch (error) { | ||
console.error("Error while fetching breadcrumbs", error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not handle errors in composable, it should be thrown to invoking component to handle in the project - analytics/notifications etc. however they need to handle it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"@shopware-pwa/composables-next": minor | ||
--- | ||
|
||
Added `getCategoryBreadcrumbs` method for fetching backend breadcrumbs in `useBreadcrumbs` composable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the buildDynamicBreadcrumbs
is more important functionality, should be mentioned and explained here more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @param page | ||
* @returns | ||
*/ | ||
export function getCmsBreadcrumbs< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this helper - it's not creating breadcrumbs, just displaying entity translated property, when would that be useful for the users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using it to build CMS page breadcrumbs, where we always have 1 level of breadcrumbs
Description
closes #139
Type of change
ToDo's
Screenshots (if applicable)
Additional context