-
Notifications
You must be signed in to change notification settings - Fork 331
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
feat: show default insight #10283
feat: show default insight #10283
Conversation
@@ -93,7 +93,7 @@ const WholeMeetingSummaryResult = ({meetingRef}: Props) => { | |||
<td | |||
align='center' | |||
style={textStyle} | |||
className='summary-link-style' |
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.
I updated this as link-style is being used in two places now, not just the summary
@@ -109,15 +110,15 @@ const TeamDashHeader = (props: Props) => { | |||
`, | |||
teamRef | |||
) | |||
const {organization, id: teamId, name: teamName, teamMembers} = team | |||
const {organization, id: teamId, name: teamName, teamMembers, isViewerTeamLead} = team |
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.
When the team feature flag is ready, I'll add a check for this too
</h2> | ||
<p className='mb-6 text-sm text-slate-600'>Summarized {insight?.meetingsCount} meetings</p> |
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.
I'll add empty state soon (#10281) so there's no need to check if insights is null
|
||
<h3 className='mb-0 text-lg font-semibold text-slate-700'>Challenges</h3> | ||
<p className='mb-2 mt-0 text-sm italic text-slate-600'> | ||
What challenges has "{name}" faced during this timeframe? |
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.
The design shows "this team" but I changed that to "name" as it's more personalised
return teamMember?.isLead || false | ||
}, | ||
insight: async ({id: teamId}, _args, {dataLoader}) => { | ||
const insight = await dataLoader.get('latestInsightByTeamId').load(teamId) |
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.
I'm getting a type error. The problem seems to be that the id is an Int because I'm using SERIAL here.
Insight.id is defined as type ID here.
But I get a type error, that's resolved when I do:
insight: async ({id: teamId}, _args, {dataLoader}) => {
const insight = await dataLoader.get('latestInsightByTeamId').load(teamId)
if (!insight) return null
return {
...insight,
id: 'hey'
}
}
So the problem seems to be the id is an int, but it's expecting a string. I could change the type Insight.id to Int here, but that doesn't seem clean when it's an ID.
Any ideas?
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.
-1 Yes, the type error is correct and caught an error. We don't want to return the database id directly (of type int) because IDs should be globally unique (something to do with Relay caching). So we should create an GQL ID type for insights where we do the usual join
/split
and something like insights:${teamId}:${databaseId}
.
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.
Ah that makes sense, thanks!
return teamMember?.isLead || false | ||
}, | ||
insight: async ({id: teamId}, _args, {dataLoader}) => { | ||
const insight = await dataLoader.get('latestInsightByTeamId').load(teamId) |
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.
-1 Yes, the type error is correct and caught an error. We don't want to return the database id directly (of type int) because IDs should be globally unique (something to do with Relay caching). So we should create an GQL ID type for insights where we do the usual join
/split
and something like insights:${teamId}:${databaseId}
.
""" | ||
If the viewer is the team lead | ||
""" | ||
isViewerTeamLead: Boolean! |
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.
+1 Is this so much shorter than
viewerTeamMember {
isLead
}
?
Let's try to not bloat the interface, just more places where we can introduce bugs.
if (!insight) return null | ||
return { | ||
...insight, | ||
id: InsightId.join(teamId, insight.id) |
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.
+1 The nicer solution would be to have a resolver for the insights type. Define the DB type as source for it and then have a single id field doing this conversion. This way it will work in all occurrences where we would add this field.
Fix #10239
You could test this PR by connecting to staging and running
generateInsight
from graphiql. However, I suggest just reviewing the code as we'll only release this to Parabol to start with once the team feature flag is ready.In the next PR, I'll create an empty state where you can generate an insight if one doesn't exist.