fix: scoping of breadcrumb links, add unified function that resolves … #3099
Conversation
demo/site/src/util/resolveUrl.ts
Outdated
| /** | ||
| * Resolves a path, scope and a baseUrl to a real URL which can be navigated to. | ||
| */ | ||
| export const resolveUrl = ({ baseUrl = "/", path, scope, anchor }: ResolveUrlOptions) => { |
There was a problem hiding this comment.
missing test setup in demo/site, would like to unit test this function.
manuelblum
force-pushed
the
fix/com-1533-breadcrumb-links-not-respecting-scope-language
branch
2 times, most recently
from
5bfefd2 to
f0d7e1c
Compare
manuelblum
force-pushed
the
fix/com-1533-breadcrumb-links-not-respecting-scope-language
branch
from
bb92f22 to
14eeac0
Compare
demo/site/src/util/resolveUrl.ts
Outdated
| * | ||
| * @return {string} The resolved URL: /{scope.language}/test/to/my/page#my-anchor | ||
| */ | ||
| export const resolveUrl = ({ baseUrl = "/", path, scope, anchor }: ResolveUrlOptions) => { |
There was a problem hiding this comment.
- baseUrl should not include trailing
/and default to "" - I would even consider removing the baseUrl from this new function and keep appending it in sitemap (as this is the only place where it is needed)
- scope.language should be required (in this multi-lang setup) - creating urls without scope.language is not possible and would result in an invalid url
- maybe rename to
createSiteUrl,resolveUrlmight be too generic
There was a problem hiding this comment.
renamed resolveUrl to createSiteUrl
There was a problem hiding this comment.
I made the scope optional because of the implementation detail of InternalLink: https://github.com/vivid-planet/comet/pull/3099/files#diff-3dfc3d13f8394017033da0b81712accc4475939f36a1df4826a6151807d8ba57R22-R26.
I think scope can be null there, at least its mentioned in the blocks.generated.ts
I am unsure about that, but I can make the scope required in this function and render the span instead.
There was a problem hiding this comment.
Concerning the baseUrl -> nearly every link I have seen in the demo is starting with a /.
There was a problem hiding this comment.
but I think it's a good idea to start with default value = empty string.
manuelblum
force-pushed
the
fix/com-1533-breadcrumb-links-not-respecting-scope-language
branch
from
a68a326 to
0b96f43
Compare
|
Description
This Pull Requests:
resolveUrlthat resolves all kinds of URLs (scoped, not scoped, anchor, baseUrl)Screenshots/screencasts
Further information