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

Require routing param for controlled pagination #2400

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

zamoore
Copy link
Contributor

@zamoore zamoore commented Sep 9, 2024

📌 Summary

If merged, this PR will require that consumers of our pagination components (
Hds::Pagination::Compact & Hds::Pagination::Numbered) provide at least 1 routing argument
(@model, @models, or @route) when routing is controlled externally.

🛠️ Detailed description

  • Added new assertions in component constructors that ensure the above
  • Added some discriminated union types to the pagination type definitions that make it more clear when certain arguments are required.

Questions

  • How to we communicate this required-if behavior to the user in the docs? Is what we have enough?
  • The discriminated union types seem to work in TS not with Glint. (IE, if I try to instantiate a pagination component with @onPageChange but omit routing params, I get no TS errors, but if I try to create an object with the same type in the component file, I get the expected errors.

🔗 External links

Jira ticket: HDS-3439


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@zamoore zamoore self-assigned this Sep 9, 2024
Copy link

vercel bot commented Sep 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 13, 2024 4:31pm
hds-website ✅ Ready (Inspect) Visit Preview Sep 13, 2024 4:31pm

@hashibot-hds hashibot-hds added packages/components docs-website Content updates to the documentation website showcase labels Sep 9, 2024
@zamoore zamoore requested a review from a team September 9, 2024 21:19
@zamoore zamoore marked this pull request as ready for review September 9, 2024 21:23
@zamoore zamoore marked this pull request as draft September 10, 2024 03:25
@zamoore zamoore removed the request for review from a team September 10, 2024 03:25
@zamoore zamoore force-pushed the 3439-assert-model-requirement branch from 220e625 to b081bfa Compare September 10, 2024 13:31
@zamoore zamoore marked this pull request as ready for review September 10, 2024 14:14
@zamoore zamoore requested a review from a team September 10, 2024 14:14
Copy link
Contributor

@shleewhite shleewhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a couple nit comments.

@alex-ju alex-ju self-requested a review September 11, 2024 15:18
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Clearer type abstractions! Aside from the warning I would also update the Component API section stating when route/model/models is required.

Would this change also apply to replace or only relevant to route/model/models?

How to we communicate this required-if behavior to the user in the docs? Is what we have enough?

I believe the warning and flagging this in the component API section is the best we can do for now

The discriminated union types seem to work in TS not with Glint. (IE, if I try to instantiate a pagination component with @onPageChange but omit routing params, I get no TS errors, but if I try to create an object with the same type in the component file, I get the expected errors.

I've been running into this as well. I don't have a solution at the moment, hopefully the documentation will help.

@zamoore
Copy link
Contributor Author

zamoore commented Sep 12, 2024

@alex-ju

Would this change also apply to replace or only relevant to route/model/models?

I don't believe that providing the replace argument should be required in any scenario. It should always be optional.

@zamoore zamoore merged commit 7252970 into main Sep 13, 2024
14 checks passed
@zamoore zamoore deleted the 3439-assert-model-requirement branch September 13, 2024 18:03
@hashibot-hds hashibot-hds mentioned this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components showcase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants