goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.22k stars 133 forks source link

feat(carousel): add carousel component #160

Closed theo-matzavinos closed 6 months ago

theo-matzavinos commented 6 months ago

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 #117

What is the new behavior?

Does this PR introduce a breaking change?

Other information

theo-matzavinos commented 6 months ago

One thing I'd might change is the directives for the buttons to become: hlmCarouselPrevious and hlmCarouselNext.

They're actually components which is why I opted for the kebab-case selector. I've borrowed this idea from material/cdk. I have no strong feelings about it though.

Some notes/questions:

  1. I didn't exactly follow shadcn's implementation and moved the scroll container to the root component. It was easier to implement this way especially since in shadcn they forward the classes to the inner div.
  2. Should I change the icons to lucide?
  3. There's really no logic since it's a helm component. Do we need to test anything?

Looking forward to your feedback.

goetzrobin commented 6 months ago

One thing I'd might change is the directives for the buttons to become: hlmCarouselPrevious and hlmCarouselNext.

They're actually components which is why I opted for the kebab-case selector. I've borrowed this idea from material/cdk. I have no strong feelings about it though.

Some notes/questions:

  1. I didn't exactly follow shadcn's implementation and moved the scroll container to the root component. It was easier to implement this way especially since in shadcn they forward the classes to the inner div.
  2. Should I change the icons to lucide?
  3. There's really no logic since it's a helm component. Do we need to test anything?

Looking forward to your feedback.

Maybe we can add both selectors? You can have multiple selectors separated by a comma.

  1. I don't have any problems with that! Seems like a reasonable change
  2. Yes please that would be great!
  3. No tests needed for helm if they don't include any logic. Maybe add a sanity e2e test that just ensures the basic functionality works?
goetzrobin commented 6 months ago

@theo-matzavinos Let me know if that gives you everything you need or if there's anything I can do to help you!

theo-matzavinos commented 6 months ago

Maybe we can add both selectors? You can have multiple selectors separated by a comma.

That's reasonable. I should use the camelCase selectors in the docs then?