Closed marcjulian closed 6 months ago
I would like your feedback @goetzrobin on how we should implement previous/next. I added it as component but now I would need to handle passing href
or routerLink
.
Open tasks
I would like your feedback @goetzrobin on how we should implement previous/next. I added it as component but now I would need to handle passing
href
orrouterLink
.Open tasks
- [ ] update storybook
- [ ] copy pagination to cli
Interesting question.... My mind went to thinking that you can just go with providing both as inputs (href & routerLink) and under the hood determine to use routerLink or href. Although if we pick one for now I'd go with routerLink as this is pretty much the default of how we link in Angular apps.
Okay i added href
as input to Next and Previous component, using routerLink
for navigation.
I guess routerLink
is the better approach anyways, the pagination probably never points to an external page.
@marcjulian looking at your example now I think we should change it to router link and also add support for the additional router link option inputs. Your argument that this will most likely always be internal navigation makes sense! Sorry about the change of mind here!
If I rename the input to routerLink
it will act as input and as directive in the preview.
The routerLink
directive will be applied to hlm-pagination-previous
and passed as input to the component and then applied to the anchor tag.
I see following options:
a
tag out from hlm-pagination-previous/next
and wrap the component in template with a
tagrouterlink
to avoid applying the directive tooWith approach 1. the user has full access to the anchor tag in the template, with 2. we would need to handle the additional router link options.
If I rename the input to
routerLink
it will act as input and as directive in the preview.
The
routerLink
directive will be applied tohlm-pagination-previous
and passed as input to the component and then applied to the anchor tag.
I see following options:
- Move
a
tag out fromhlm-pagination-previous/next
and wrap the component in template witha
tag- Rename to
routerlink
to avoid applying the directive tooWith approach 1. the user has full access to the anchor tag in the template, with 2. we would need to handle the additional router link options.
@marcjulian Very good points. Maybe we stay with href or just do [link]="#" [linkOptions]="{ROUTER_OPTIONS_HERE}" ? I am open to hear your suggestion! We would also make routerlink a host directive for hlmPaginationLink and keep the same [link]="#" [linkOptions]="{ROUTER_OPTIONS_HERE}" inputs that are forwarded to routerlink? That way the API stays consitent?
@goetzrobin the idea with RouterLink
as host directive for HlmPaginationLinkDirective
is a really good idea! Haven't used it yet, really powerful!
I would prefer going with link
as input for routerLink
, as it is closer than href
.
Which router link options do you mean should be exposed with linkOptions
as input? Should we add all RouterLink inputs to the host directive and keeping the name?
For example
@Directive({
selector: '[hlmPaginationLink]',
standalone: true,
hostDirectives: [
{
directive: RouterLink,
inputs: ['routerLink: link', 'queryParams', 'fragment' ...], // 👈
},
],
host: {
'[class]': '_computedClass()',
'[attr.aria-current]': 'isActive() ? "page" : null',
},
})
export class HlmPaginationLinkDirective {
@marcjulian I'd say we keep all of them! Let's do link! I like it!!
I run prepare-cli
to copy hlm to cli. HlmPaginationLinkDirective
uses buttonVariants
, relying on the button component. Do I need to add this relationship somewhere to the CLI or how is it handled?
You probably need to adjust this part of the CLI: https://github.com/goetzrobin/spartan/blob/main/libs/cli/src/generators/base/lib/build-dependency-array.ts Similar to DEPENDENT_ON_DIALOG
@goetzrobin I think I have found it. Please try it out and let me know what to do next.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Which package are you modifying?
What is the current behavior?
Closes #119
What is the new behavior?
Does this PR introduce a breaking change?
Other information