Skip to content

feat(roles): Info tooltip for org-roles#46613

Merged
leedongwei merged 9 commits into
masterfrom
dlee/roles-tooltip
Mar 31, 2023
Merged

feat(roles): Info tooltip for org-roles#46613
leedongwei merged 9 commits into
masterfrom
dlee/roles-tooltip

Conversation

@leedongwei

@leedongwei leedongwei commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

You can get org-roles from several sources so we need a common component to bring people into docs for a thorough explanation.

Screenshot 2023-03-30 at 6 10 36 PM

@leedongwei leedongwei requested a review from a team March 30, 2023 17:55
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 30, 2023

@nhsiehgit nhsiehgit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

This also feels like a lot of content for just a tooltip 🤔

Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx Outdated
@leedongwei leedongwei marked this pull request as ready for review March 31, 2023 01:15
@leedongwei leedongwei requested a review from a team as a code owner March 31, 2023 01:15
Comment thread static/app/components/orgRole.tsx
Comment thread static/app/components/orgRole.tsx
Comment thread static/app/components/orgRole.tsx Outdated
Comment on lines +34 to +41
if (!orgRoleFromMember) {
Sentry.withScope(scope => {
scope.setExtra('context', {
memberId: member.id,
orgRole: member.orgRole,
});
Sentry.captureException(new Error('OrgMember has an invalid orgRole.'));
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want this to be in a useEffect right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no side-effects here. This component doesn't maintain state or change state.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sentry.captureExceotion is a side effect, we don’t want that firing when the component rerenders

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea, I understand now. Fixed.

Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx
Comment thread static/app/components/orgRole.tsx Outdated
Comment thread static/app/components/orgRole.tsx
Comment thread static/app/components/orgRole.tsx Outdated
leedongwei and others added 3 commits March 30, 2023 22:31
Co-authored-by: Evan Purkhiser <evanpurkhiser@gmail.com>
Co-authored-by: Evan Purkhiser <evanpurkhiser@gmail.com>

@evanpurkhiser evanpurkhiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smash it in danny

@leedongwei leedongwei enabled auto-merge (squash) March 31, 2023 20:15
@leedongwei leedongwei merged commit 4d6cb0b into master Mar 31, 2023
@leedongwei leedongwei deleted the dlee/roles-tooltip branch March 31, 2023 20:23
@leedongwei leedongwei mentioned this pull request Mar 31, 2023
1 task
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants