Skip to content
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

fix: SSO button link target [DET-10171] #8925

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Conversation

julian-determined-ai
Copy link
Contributor

@julian-determined-ai julian-determined-ai commented Feb 29, 2024

Description

See: https://hpe-aiatscale.slack.com/archives/CLCE8D998/p1709221849482189

The link for SSO did not wrap entirely around the button element. This fixes the issue.

Test Plan

  1. Ensure that your UI is pointing to an SSO enabled cluster. For example our gcloud cluster.
  2. On the sign in page ensure that clicking anywhere on the SSO sign in button sends you to the appropriate login page.

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

Copy link

netlify bot commented Feb 29, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 5a0de8d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65e0b2899105ef00087f9623
😎 Deploy Preview https://deploy-preview-8925--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@julian-determined-ai julian-determined-ai added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Feb 29, 2024
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

lgtm -- looking forward, we really should have link buttons as part of the ui kit.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 42.45%. Comparing base (5367f4f) to head (5a0de8d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8925      +/-   ##
==========================================
- Coverage   47.17%   42.45%   -4.72%     
==========================================
  Files        1154      840     -314     
  Lines      175088   136240   -38848     
  Branches     2237     2235       -2     
==========================================
- Hits        82596    57845   -24751     
+ Misses      92334    78237   -14097     
  Partials      158      158              
Flag Coverage Δ
harness ?
web 42.53% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
webui/react/src/pages/SignIn.tsx 0.00% <0.00%> (ø)

... and 314 files with indirect coverage changes

@julian-determined-ai julian-determined-ai changed the title fix: SSO button link target fix: SSO button link target [DET-10171] Feb 29, 2024
@julian-determined-ai julian-determined-ai merged commit beac348 into main Feb 29, 2024
83 of 95 checks passed
@julian-determined-ai julian-determined-ai deleted the det-10171 branch February 29, 2024 16:55
dai-release bot pushed a commit that referenced this pull request Feb 29, 2024
(cherry picked from commit beac348)
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants