feat: switch template UI #9093
feat: switch template UI #9093
Conversation
| @@ -174,6 +174,7 @@ const User: UserResolvers = { | |||
| ...activity, | |||
| sortOrder: getScore(activity, teamIds) | |||
| })) | |||
| .filter((activity) => !type || activity.type === type) | |||
There was a problem hiding this comment.
I could add type to the dataloader (or create a second dataloader) so that we only get retrospective templates. But as this query is used in the activity library, which will probably be executed for the org before the user gets to the retro drawer, it should be more efficient to keep it as it is and get the templates from the cache.
There was a problem hiding this comment.
I agree that we don't need another dataloader, but I don't really understand the rationale. The dataloader cache is only valid for the duration of one query and the corresponding subscription notifications.
Instead if we wouldn't include this type filter and instead do all filtering on the client side, there is the possibility that relay reads it from its cache.
| <Menu | ||
| trigger={ | ||
| <OptionsButton> | ||
| <IconLabel icon='tune' iconLarge /> | ||
| <div className='text-slate-700'>Options</div> | ||
| </OptionsButton> | ||
| } | ||
| > | ||
| <div ref={originRef} onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave}> | ||
| <MenuItem | ||
| value='template' | ||
| label='Change template' | ||
| onClick={handleClick} | ||
| isDisabled={hasReflections} | ||
| icon={<SwapHorizIcon />} | ||
| /> | ||
| </div> | ||
| {tooltipPortal('You can only change the template before reflections have been added.')} | ||
| </Menu> |
There was a problem hiding this comment.
+1 I would ideally keep radix structure rather than passing components as props
<Menu>
<MenuTrigger>
<OptionsButton>
<IconLabel icon='tune' iconLarge />
<div className='text-slate-700'>Options</div>
</OptionsButton>
</MenuTrigger>
<MenuContent>
<MenuItem>Change template</MenuItem>
</MenuContent>
</Menu>
There was a problem hiding this comment.
I prefer including trigger as a prop because it's always required, and it means the dev just needs to import Menu & MenuItem. I've removed the icon as a prop though
|
We don't need a feature flag here? |
Nope, I suggested that we don't use a feature flag here and just roll it out to all users. I won't merge this PR until the logic is in place though. |
| @@ -174,6 +174,7 @@ const User: UserResolvers = { | |||
| ...activity, | |||
| sortOrder: getScore(activity, teamIds) | |||
| })) | |||
| .filter((activity) => !type || activity.type === type) | |||
There was a problem hiding this comment.
I agree that we don't need another dataloader, but I don't really understand the rationale. The dataloader cache is only valid for the duration of one query and the corresponding subscription notifications.
Instead if we wouldn't include this type filter and instead do all filtering on the client side, there is the possibility that relay reads it from its cache.
| </TooltipTrigger> | ||
| </div> | ||
| <TooltipContent> | ||
| {'You can only change the template before reflections have been added.'} |
There was a problem hiding this comment.
+1 I would rephrase this slightly to "You can only change the template if no reflections have been added." because it also works if you remove them again.
|
@Dschoordsch this PR is relevant for recurring retros. It shouldn't be merged until #9088 is ready as it'd be confusing to have the UI without the functionality. Would you like to work on this as you're working on recurring retros now? |
Fix #9087
Demo: https://www.loom.com/share/758f0aef94a943dc9ec28009125b325c
This PR won't be merged until the functionality is added in a subsequent PR.
I'm using the "Options" button from Standups. One small UI thing is the avatar list gets larger and smaller depending on the screen size whereas "options" remains the same height. As that affects Standups, I'll tackle it in a separate PR.
I created a reusable menu & menu item component in this PR:
To test