-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(console): Update request console sdk access modal and button #105612
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
base: alexsohn/feat/add-github-identity-and-provider-factory-for-testing
Are you sure you want to change the base?
Changes from all commits
c2db8e6
7dc636c
2fb9e1b
06a0c19
3f10cfd
aeaacaf
bd45b00
9b73a8b
555d43f
aab48f2
3333b1a
c54d601
4757a0d
13ff7b2
9dccfba
21242b6
e7d61a6
98f4ce0
a28b64a
0577ffd
b3a5f59
9c9606c
5e39637
410a905
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import {Button} from '@sentry/scraps/button'; | ||
|
|
||
| import {openPrivateGamingSdkAccessModal} from 'sentry/actionCreators/modal'; | ||
| import type {PrivateGamingSdkAccessModalProps} from 'sentry/components/modals/privateGamingSdkAccessModal'; | ||
| import {IconLock} from 'sentry/icons'; | ||
| import {t} from 'sentry/locale'; | ||
| import {useReopenGamingSdkModal} from 'sentry/utils/useReopenGamingSdkModal'; | ||
|
|
||
| export function RequestSdkAccessButton({ | ||
| gamingPlatform, | ||
| organization, | ||
| origin, | ||
| projectId, | ||
| }: Omit<PrivateGamingSdkAccessModalProps, 'onSubmit'>) { | ||
| const buttonProps: PrivateGamingSdkAccessModalProps = { | ||
| gamingPlatform, | ||
| organization, | ||
| origin, | ||
| projectId, | ||
| }; | ||
|
|
||
| useReopenGamingSdkModal(buttonProps); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do understand why it makes sense to use the hook here, but in my opinion it should be used outside of this component. I wouldn’t expect a button component to automatically open a modal just by being rendered. Does that make sense?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I completely get that, I thought I was being smart by doing this, but I can see why this would be confusing for whoever uses this component. I'll move these hooks out to the pages they are being used in 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iiuc it doesn't automatically open the modal right? it still requires the click to open it. What it does do is open the modal if there's a specific query param int he URL. If that's the case, I think it makes sense to have it in here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @billyvg Yea, it only opens the modal if a specific query param in the URL!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been looking at how I can extract out the hook, but it feels like maybe leaving a comment on the component would be a better fix as this button won't be super widely used.. LMK what you think @priscilawebdev |
||
|
|
||
| return ( | ||
| <Button | ||
| priority="primary" | ||
| size="sm" | ||
| data-test-id="request-sdk-access" | ||
| icon={<IconLock locked />} | ||
| onClick={() => { | ||
| openPrivateGamingSdkAccessModal(buttonProps); | ||
| }} | ||
| > | ||
| {t('Request SDK Access')} | ||
| </Button> | ||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.