-
Notifications
You must be signed in to change notification settings - Fork 20
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
Pressroom releases #295
Pressroom releases #295
Conversation
Branch preview✅ Deployed successfully in branch deployment: |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
9efd323
to
3ec505b
Compare
onClick={() => handleToggleCategory(category)} | ||
> | ||
<Typography> | ||
{category} | ||
{isSelected && ( | ||
<IconButton className={css.closeFilter} onClick={() => handleToggleCategory(category)}> |
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.
Nit: we can remove the duplication by creating a handler for () => handleToggleCategory(category)
.
|
||
const PAGE_QUERY_PARAM = 'page' | ||
|
||
export const getPage = (query: NextRouter['query']): number => { |
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.
Nit: I'd suggest moving this to a utility file as it is used in multiple components.
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.
Thanks for catching this! I wanted to extract it to an util function but forgot.
const handleToggleCategory = (category: string) => () => { | ||
const queryParams = { ...router.query } | ||
|
||
if (queryParams.category === category) { | ||
delete queryParams.category | ||
} else { | ||
queryParams.category = category | ||
} | ||
|
||
router.push( | ||
{ | ||
query: queryParams, | ||
}, | ||
undefined, | ||
{ shallow: true }, | ||
) | ||
} | ||
|
||
return ( | ||
<Grid container className={css.filterWrapper}> | ||
{categories.map((category) => { | ||
const isSelected = category === selectedCategory | ||
|
||
return ( | ||
<Grid item key={category} className={css.filterCard} xs={12} md="auto"> | ||
<ButtonBase | ||
className={clsx(css.filterButton, { [css.selected]: isSelected })} | ||
onClick={handleToggleCategory(category)} | ||
> | ||
<Typography> | ||
{category} | ||
{isSelected && ( | ||
<IconButton className={css.closeFilter} onClick={handleToggleCategory(category)}> |
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.
Currying this still creates the function twice. You can create it within the map
once, e.g.
const handleToggleCategory = (category: string) => () => { | |
const queryParams = { ...router.query } | |
if (queryParams.category === category) { | |
delete queryParams.category | |
} else { | |
queryParams.category = category | |
} | |
router.push( | |
{ | |
query: queryParams, | |
}, | |
undefined, | |
{ shallow: true }, | |
) | |
} | |
return ( | |
<Grid container className={css.filterWrapper}> | |
{categories.map((category) => { | |
const isSelected = category === selectedCategory | |
return ( | |
<Grid item key={category} className={css.filterCard} xs={12} md="auto"> | |
<ButtonBase | |
className={clsx(css.filterButton, { [css.selected]: isSelected })} | |
onClick={handleToggleCategory(category)} | |
> | |
<Typography> | |
{category} | |
{isSelected && ( | |
<IconButton className={css.closeFilter} onClick={handleToggleCategory(category)}> | |
const handleToggleCategory = (category: string) => { | |
const queryParams = { ...router.query } | |
if (queryParams.category === category) { | |
delete queryParams.category | |
} else { | |
queryParams.category = category | |
} | |
router.push( | |
{ | |
query: queryParams, | |
}, | |
undefined, | |
{ shallow: true }, | |
) | |
} | |
return ( | |
<Grid container className={css.filterWrapper}> | |
{categories.map((category) => { | |
const isSelected = category === selectedCategory | |
const handleToggle = () => handleToggleCategory(category) | |
return ( | |
<Grid item key={category} className={css.filterCard} xs={12} md="auto"> | |
<ButtonBase | |
className={clsx(css.filterButton, { [css.selected]: isSelected })} | |
onClick={handleToggle} | |
> | |
<Typography> | |
{category} | |
{isSelected && ( | |
<IconButton className={css.closeFilter} onClick={handleToggle}> |
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.
WDYT of memoizing the function with useCallback
?
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.
Moved the handler inside of the mapping function
What it solves
Adds the Press releases section on the Pressroom page
Refactors the quick categories filter to its own component in the
/common
folderScreenshots
Figma
https://www.figma.com/file/zNUgOvD1WcFankfXfBiAfs/Press-room?type=design&node-id=9929-4249&mode=design&t=x13Zf5TNtfe0ytJN-0