-
Notifications
You must be signed in to change notification settings - Fork 1
Events group page and table #669
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
Conversation
|
Preview deployment: https://events-table.preview.avy-fx.org |
ad49cb9 to
57afa28
Compare
busbyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback on these changes.
src/app/api/events/route.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing missing from the existing https://github.com/NWACus/web/blob/events/src/app/api/%5Bcenter%5D/events/route.ts that you have here is a depth query param. So could you just add that there if you need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to consolidate these files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only #669 (comment) but yea that's what I was suggesting.
|
|
||
| const conditions: Where[] = [ | ||
| { | ||
| 'tenant.slug': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use https://github.com/NWACus/web/blob/events/src/app/api/%5Bcenter%5D/events/route.ts you can keep using slug because we have the center slug as a route param (which the events page expects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@busbyk We can sadly only get the tenant id from src/fields/EventQuery/QueriedEventsComponent.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. OK well this change (the expectation that the center param is an id not a slug) breaks src/app/(frontend)/[center]/events/page.tsx because it gets center from the route param which is a slug, not an id. getEvents is used there too.
I agree that it would be difficult to get slug not id in src/fields/EventQuery/QueriedEventsComponent.tsx. But here's the thing: do we even need src/fields/EventQuery/QueriedEventsComponent.tsx anymore? Since we're dynamically fetching events in EventList and EventTable, does it really make sense to show what events will show up when that might change based on when a visitor is visiting the site?
If we did want to keep a preview of what events will be shown, we could use a virtual field instead since the results don't need to be persisted for revalidation anymore. But we would still have the issue of needing to filter by tenant id not slug. So removing src/fields/EventQuery/QueriedEventsComponent.tsx would solve that problem for us. And it's pretty reasonable for a user to preview what events would be shown by opening the live preview view like they do for other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right in making Preview do the work. We don't do that with any other blocks. I did need to update EventListBlock to use the endpoint since it was relying on queriedEvents, but that is fine.
We're duplicating the fetchEvents data fetch across these two files (table and list). Should we extract this into a custom hook like, useFetchEvents so both files can share it? This would reduce duplication and make future updates easier, but I don't know if we want to start this pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the same component for EventList and EventTable with an extra prop that dictates whether to render the event list UI or the event table UI?
57afa28 to
5f2d833
Compare
busbyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main issue is that the /events page is now broken because of the mismatch between tenant slug and id expected in getEvents.
|
|
||
| const conditions: Where[] = [ | ||
| { | ||
| 'tenant.slug': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. OK well this change (the expectation that the center param is an id not a slug) breaks src/app/(frontend)/[center]/events/page.tsx because it gets center from the route param which is a slug, not an id. getEvents is used there too.
I agree that it would be difficult to get slug not id in src/fields/EventQuery/QueriedEventsComponent.tsx. But here's the thing: do we even need src/fields/EventQuery/QueriedEventsComponent.tsx anymore? Since we're dynamically fetching events in EventList and EventTable, does it really make sense to show what events will show up when that might change based on when a visitor is visiting the site?
If we did want to keep a preview of what events will be shown, we could use a virtual field instead since the results don't need to be persisted for revalidation anymore. But we would still have the issue of needing to filter by tenant id not slug. So removing src/fields/EventQuery/QueriedEventsComponent.tsx would solve that problem for us. And it's pretty reasonable for a user to preview what events would be shown by opening the live preview view like they do for other changes.
src/app/api/events/route.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only #669 (comment) but yea that's what I was suggesting.
|
|
||
| const conditions: Where[] = [ | ||
| { | ||
| 'tenant.slug': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the same component for EventList and EventTable with an extra prop that dictates whether to render the event list UI or the event table UI?
Description
This PR adds the ability to add a page for event group and adds an event table
Related Issues
Fixes #632
Key Changes
/events/g/[slug]gfrom UIEventListandEventTableScreenshots / Demo
Migration Explanation
Yes - but done in #635
Future enhancements / Questions
TODO: Tablet view of events group