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

Change CLI auth pages #4638

Closed
wants to merge 1 commit into from
Closed

Change CLI auth pages #4638

wants to merge 1 commit into from

Conversation

davejcameron
Copy link
Contributor

@davejcameron davejcameron commented Oct 11, 2024

WHY are these changes introduced?

The current CLI auth pages are a green full screen with simple test. This doesn't match any of the colours we currently use so I wanted to just make it a card.

WHAT is this pull request doing?

Before After
image image

How to test your changes?

Open the files up on your local machine, they are static

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@davejcameron davejcameron requested a review from a team as a code owner October 11, 2024 13:14
Copy link
Contributor

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/app-inner-loop

Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.6% (+0.01% 🔼)
8498/11705
🟡 Branches
69.59% (+0.02% 🔼)
4174/5998
🟡 Functions 71.73% 2207/3077
🟡 Lines
72.93% (+0.01% 🔼)
8041/11026

Test suite run success

1933 tests passing in 873 suites.

Report generated by 🧪jest coverage report action from 358842a

@eytanseidman15
Copy link

I discussed with @davejcameron and supportive of doing this but the look and feel should be that of our account / identity layer because that's really where this sits. cc: @nickwesselman

@amcaplan
Copy link
Contributor

When do we still use these screens? We should be using device auth by default now.

@isaacroldan
Copy link
Contributor

We don't use these screens anymore, in fact the plan is to remove all of these soon, while removing the legacy auth flow.

@gonzaloriestra
Copy link
Contributor

Right, they are being removed here: #4604

@shauns
Copy link
Contributor

shauns commented Oct 15, 2024

Yep. We can close this PR -- device auth has been default on recent CLIs for a while and uses Shopify hosted and branded pages, and the auth flow that shows this screen is now fully deprecated. Thank you though!

@shauns shauns closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants