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(pagination): add pagination #142

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

marcjulian
Copy link
Contributor

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
  • calendar
  • card
  • checkbox
  • collapsible
  • combobox
  • command
  • context-menu
  • data-table
  • date-picker
  • dialog
  • dropdown-menu
  • hover-card
  • input
  • label
  • menubar
  • navigation-menu
  • pagination
  • popover
  • progress
  • radio-group
  • scroll-area
  • select
  • separator
  • sheet
  • skeleton
  • slider
  • switch
  • table
  • tabs
  • textarea
  • toast
  • toggle
  • tooltip
  • typography

What is the current behavior?

Closes #119

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@marcjulian
Copy link
Contributor Author

marcjulian commented Jan 31, 2024

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

  •  update storybook
  • copy pagination to cli

@goetzrobin
Copy link
Owner

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

  •  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.

@marcjulian
Copy link
Contributor Author

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.

@goetzrobin
Copy link
Owner

@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!

@marcjulian
Copy link
Contributor Author

If I rename the input to routerLink it will act as input and as directive in the preview.

CleanShot 2024-02-01 at 10 38 12

The routerLink directive will be applied to hlm-pagination-previous and passed as input to the component and then applied to the anchor tag.

CleanShot 2024-02-01 at 10 39 12

I see following options:

  1. Move a tag out from hlm-pagination-previous/next and wrap the component in template with a tag
  2. Rename to routerlink to avoid applying the directive too

With 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.

@goetzrobin
Copy link
Owner

If I rename the input to routerLink it will act as input and as directive in the preview.

CleanShot 2024-02-01 at 10 38 12

The routerLink directive will be applied to hlm-pagination-previous and passed as input to the component and then applied to the anchor tag.

CleanShot 2024-02-01 at 10 39 12

I see following options:

  1. Move a tag out from hlm-pagination-previous/next and wrap the component in template with a tag
  2. Rename to routerlink to avoid applying the directive too

With 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?

@marcjulian
Copy link
Contributor Author

@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 {

@goetzrobin
Copy link
Owner

@marcjulian I'd say we keep all of them! Let's do link! I like it!!

@marcjulian
Copy link
Contributor Author

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?

@goetzrobin
Copy link
Owner

goetzrobin commented Feb 2, 2024

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

@marcjulian
Copy link
Contributor Author

@goetzrobin I think I have found it. Please try it out and let me know what to do next.

@goetzrobin goetzrobin merged commit 0f118cb into goetzrobin:main Feb 13, 2024
5 of 7 checks passed
@marcjulian marcjulian deleted the feature/pagination branch February 13, 2024 16:20
ty-ler pushed a commit to ty-ler/spartan that referenced this pull request Feb 28, 2024
* feat(pagination): add pagination

* feat(pagination): add href to previous/next and use routerLink for navigation

* feat(pagination): update storybook

* feat(pagination): use routerLink as host directive for paginationLink

* rename routerLink to link as input

* feat(pagination): expose all routerLink inputs for pagination link

* feat(pagination): run hlm-to-cli generator

* feat(pagination): add pagination on roadmap

* feat(pagination): adds icon and button dependent

* feat(pagination): add to pr and issue templates
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.

RFC: Pagination
2 participants