-
Notifications
You must be signed in to change notification settings - Fork 23
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: build OKTA form within OKTA access tab #2974
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #2974 +/- ##
==========================================
- Coverage 98.36% 98.36% -0.01%
==========================================
Files 892 895 +3
Lines 13267 13304 +37
Branches 3534 3547 +13
==========================================
+ Hits 13050 13086 +36
- Misses 213 214 +1
Partials 4 4
... and 2 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2974 +/- ##
==========================================
- Coverage 98.36% 98.36% -0.01%
==========================================
Files 892 895 +3
Lines 13267 13304 +37
Branches 3515 3482 -33
==========================================
+ Hits 13050 13086 +36
- Misses 213 214 +1
Partials 4 4
... and 2 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #2974 +/- ##
==========================================
- Coverage 98.36% 98.36% -0.01%
==========================================
Files 892 895 +3
Lines 13267 13304 +37
Branches 3469 3547 +78
==========================================
+ Hits 13050 13086 +36
- Misses 213 214 +1
Partials 4 4
... and 2 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #2974 +/- ##
===========================================
Coverage 98.36000 98.36000
===========================================
Files 892 895 +3
Lines 13267 13304 +37
Branches 3534 3482 -52
===========================================
+ Hits 13050 13086 +36
- Misses 213 214 +1
Partials 4 4
... and 2 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 15.74kB ⬆️
|
Bundle ReportChanges will increase total bundle size by 15.74kB ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
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.
Couple of things to take a peak at
<TextInput | ||
{...register('clientSecret', { required: true })} | ||
type={showPassword ? 'text' : 'password'} | ||
id="clientSecret" | ||
placeholder="Enter Client Secret" | ||
/> |
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.
👏 👏 👏
} | ||
|
||
return ( | ||
<div className="w-5/6 border-2 border-solid border-ds-gray-primary p-4 text-ds-gray-octonary"> |
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.
m
: Can we move this over to our Card
component that we have? You should be able to compose/customize it as needed to match the styles given in the designs.
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.
You can check the stories for it for guidance or, poke @spalmurray-codecov if you have any deeper questions about it.
{formState.errors.clientSecret && ( | ||
<p className="mt-1 text-codecov-red"> | ||
{formState.errors.clientSecret.message} | ||
</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.
m
: Can we swap these to ternary checks:
{formState.errors.clientSecret ? (
<p className="mt-1 text-codecov-red">
{formState.errors.clientSecret.message}
</p>
) : null}
They add in some extra protections that double &
do not, if you want you can read more about these here.
{formState.errors.redirectUri && ( | ||
<p className="mt-1 text-codecov-red"> | ||
{formState.errors.redirectUri.message} | ||
</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.
m
: Same here for the swap to ternaries 👀
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.
Approved, can you add a link to the issue in the PR description.
Description
Pure UI no api stuff as they're not ready. Enforce should only be toggled on when enabled is on.
Notable Changes
Screenshots
Design: https://www.figma.com/design/rKpoilbmgXLtkZ16ADEa8P/GH-763?node-id=5-1117&t=diC5MJVu2pHoi8yl-0
codecov/engineering-team#1979
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.