Working group "About" tab description and routing #4771
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| export const WorkingGroupTabs = () => { | ||
| const { name } = useParams<{ name: string }>() | ||
| const tabs = usePageTabs(workingGroupTabs.map(([title, path]) => [title, path.replace(':name', name)])) |
There was a problem hiding this comment.
| const tabs = usePageTabs(workingGroupTabs.map(([title, path]) => [title, path.replace(':name', name)])) | |
| const tabs = usePageTabs(workingGroupTabs.map(([title, path]) => [title, generatePath(path, { name })])) |
| export const WorkingGroupPageHeader = React.memo(({ withButtons = false }: WorkingGroupPageHeaderProps) => { | ||
| const { name } = useParams<{ name: string }>() | ||
| const { group } = useWorkingGroup({ name: urlParamToWorkingGroupId(name) }) |
There was a problem hiding this comment.
useWorkingGroup is called by all WorkingGroup, WorkingGroupHistory, and WorkingGroupOpenings so it's called twice on each tab. It's not a very expensive query but I think it's not cached rn so it would be better to just pass group as a property instead:
| export const WorkingGroupPageHeader = React.memo(({ withButtons = false }: WorkingGroupPageHeaderProps) => { | |
| const { name } = useParams<{ name: string }>() | |
| const { group } = useWorkingGroup({ name: urlParamToWorkingGroupId(name) }) | |
| export const WorkingGroupPageHeader = React.memo(({ group, withButtons = false }: WorkingGroupPageHeaderProps) => { |
| @@ -53,7 +53,7 @@ const WG_JSON_OPENING = { | |||
|
|
|||
| export default { | |||
| title: 'Pages/Working Group/WorkingGroup', | |||
| component: WorkingGroup, | |||
| component: WorkingGroupOpenings, | |||
There was a problem hiding this comment.
Please leave:
| component: WorkingGroupOpenings, | |
| component: WorkingGroup, |
Because the stories in app/pages should test page behaviors
| @@ -64,7 +64,7 @@ export default { | |||
| }, | |||
|
|
|||
| parameters: { | |||
| router: { path: '/:name', href: `/${WG_DATA.name}` }, | |||
| router: { path: '/:name/openings', href: `/${WG_DATA.name}/openings` }, | |||
There was a problem hiding this comment.
For the same reason I think it would be good set the default route to:
| router: { path: '/:name/openings', href: `/${WG_DATA.name}/openings` }, | |
| router: { path: '/working-groups/:name', href: `/working-groups/${WG_DATA.name}` }, |
It creates a smoke test for the about tab. Although you might need to add some gql.queries in case the story crashes. The missing queries are shown in the console
(but no need to add everything just enough for story not to crash).
Then for the 2 tests you can set:
export const CreateOpeningImport: Story = {
parameters: {
router: { path: '/working-groups/:name', href: `/working-groups/${WG_DATA.name}` },
},
play: async ({ args, canvasElement, step }) => { ...and same thing for CreateOpeningImport.
| <MarkdownPreview markdown={workingGroup.about || defaultDescription} /> | ||
| {handbookLink && ( | ||
| <ExternalLinkButtonGhost size="small" href={handbookLink} disabled={false} target="_blank"> | ||
| Learn more from Knowlage Book |
There was a problem hiding this comment.
| Learn more from Knowlage Book | |
| Learn more from the Handbook |
There was a problem hiding this comment.
My suggestion here was wrong since the WorkingGroup component is now actually the About tab. What you implemented instead works but it duplicates a lot of code for testing just one page.
Instead I replaced WorkingGroup by WorkingGroupsModule which works more like the original suggestion. But I wanted to make sure it worked first so I committed it myself.
The rest LGTM !
Cool, thanks! |
The working "About" tab is empty (for all working groups) #4654