feat: make first tab active by default #19315
Conversation
|
@retrogtx is attempting to deploy a commit to the cal-staging Team on Vercel. A member of the Team first needs to authorize it. |
keithwillcode
added
the
community-interns
dosubot
bot
added
event-types
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (02/17/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (02/17/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (02/18/25)1 label was added to this PR based on Keith Williams's automation. |
PeerRich
added
Low priority
| avatar: item.profile.image, | ||
| "data-testid": item.profile.name ?? "", | ||
| })); | ||
| const tabs = eventTypeGroups.map((item, index) => { |
There was a problem hiding this comment.
If eventTypeGroups is large (which is expected in most cases) , wrapping the mapping logic in useMemo could improve performance.
There was a problem hiding this comment.
added a useMemo hook, got an err with that due to early return. but will implement this soon, thanks
| let href = item.teamId ? `/event-types?teamId=${item.teamId}` : "/event-types?noTeam"; | ||
| // If it's the first tab and no teamId is in the URL, set href to just /event-types |
There was a problem hiding this comment.
Let's remove the noTeam parameter. I don't think it has any use case. Removing it will fix the issue without needing any other changes."
There was a problem hiding this comment.
its because when I pressed the first team member by default, ?noTeam was in the parameter
will implement this too!
There was a problem hiding this comment.
Just make sure ?noTeam is not required else where
There was a problem hiding this comment.
upon removing the parameter, it selects both of it upon clicking another team
the parameter must stay ig
There was a problem hiding this comment.
That's because useUrlMatchesCurrentUrl uses includes and not exact matching.
If this is required we can do something like :
const isCurrent = useUrlMatchesCurrentUrl("eventTypes",href) || props?.isActive;
type TypeAllowedValues = "eventTypes";
export const useUrlMatchesCurrentUrl = (type?: TypeAllowedValues, url: string)
if (type === "eventTypes") {
return pathnameWithQuery ? pathnameWithQuery === url : false;
}
return pathnameWithQuery ? pathnameWithQuery.includes(url) : false;
There was a problem hiding this comment.
Just make sure
?noTeamis not required else where
if you click the first tab to make it active, you will get that in url parameter by default
so to make it active I just update the url with it
try it out
There was a problem hiding this comment.
Yeah , but the approach I shared above will work without ?noTeam , without affecting other places.
But it would require some prop drilling
There was a problem hiding this comment.
That's because
useUrlMatchesCurrentUrlusesincludesand not exact matching. If this is required we can do something like :
const isCurrent = useUrlMatchesCurrentUrl("eventTypes",href) || props?.isActive;type TypeAllowedValues = "eventTypes"; export const useUrlMatchesCurrentUrl = (type?: TypeAllowedValues, url: string)if (type === "eventTypes") { return pathnameWithQuery ? pathnameWithQuery === url : false; } return pathnameWithQuery ? pathnameWithQuery.includes(url) : false;
will adding exact matching like this not going to cause issues when applying filters later on
There was a problem hiding this comment.
No , but it may add bit of complications , btw I was just referring to the root cause.
Also I think the current implementation is fine
There was a problem hiding this comment.
useMemo has been added btw
|
|
E2E results are ready! |
* feat: make first tab active by default * perf: optimize event types tabs rendering with useMemo --------- Co-authored-by: Tushar Bhatt <[email protected]>
What does this PR do?
Makes the first tab active by default for teams in event types