-
Notifications
You must be signed in to change notification settings - Fork 0
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
News List #66
News List #66
Conversation
(cherry picked from commit c148540)
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.
this is awesome, David. this has the features we're after and the implementation is solid. if there was a place to call out that has the most room for improvement, i'd say it's news/index.js, purely because there's a lof of functionality baked into the page component. i think long-term maintenance would be easier if the business logic for the query params/tags was extracted to its own hook, like useQueryTags or similar.
in any event, this is all great 👍. i left a few nitpicky comments, but nothing that absolutely needs to be changed before merging. i know this feature will continue to see activity as other things fall into place.
@@ -0,0 +1,36 @@ | |||
import { Button, ButtonGroup } from "@mui/material"; | |||
|
|||
export const NewsOrFeatureToggle = ({ newsOrFeature, setNewsOrFeature }) => ( |
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.
should this be FeatureOrBlogToggle or BlogOrFeatureToggle? perhaps better would be to not bake the options into the function name. i'd suggest something a bit more generic, like ArticleTypeToggle or something similar.
Important
As the tag endpoint is still finalized, you currently need to use a plugin like ModHeader to allow the client to access
api.renci.org/graphql
, by providing a Strapi API token in theAuthorization
header:I think we'll want to wait until that endpoint is finalized before merging this, but we can go ahead testing this UI in the meantime.
features:
Waterfall skeleton loaders, query params, custom search with highlighted titles, pagination:
Screen.Recording.2023-11-10.at.12.48.04.PM.mov
Clickable tags, using browser back button to update to move state back, going back has no loader as the request has been cached:
Screen.Recording.2023-11-10.at.12.49.50.PM.mov
Mobile view, drawer opened by button with badge showing number of active filters, "no results" text:
Screen.Recording.2023-11-10.at.12.53.51.PM.mov