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

When granting consent, immediately get access code rather than polling #116

Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Sep 20, 2023

Using https://github.com/chromaui/chromatic/pull/7740, immediately on receiving the grant event, request a token from the server.

This partially solves AP-3549

📦 Published PR as canary version: 0.0.95--canary.116.adc0530.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.95--canary.116.adc0530.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.95--canary.116.adc0530.0

@linear
Copy link

linear bot commented Sep 20, 2023

AP-3549 Onboarding flow for new users to create a project

https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=1322-375121&mode=design&t=N1DXZ15edlIZ8rNJ-0

As discussed with m - the basic flow is:

  1. After signin / sign up
    1. if a user is a member of project (either because they've used chromatic before or they are in an org with projects)
      1. close the modal
      2. show the project chooser
      3. when chosen, go to step 4.
  2. Otherwise, take them to the chromatic "create first project" onboarding screen
  3. After they have created a project close the modal
  4. Show the "project selected, now create first build" flow.

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

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

This all looks pretty solid. Some tests?

@tmeasday
Copy link
Member Author

tmeasday commented Sep 21, 2023

This all looks pretty solid. Some tests?

Fair question. I'm not sure exactly what to test though.

Basically there are two functions in this file that put together REST requests in a slightly complicated way. Do we want to snapshot the data on those requests? I'm not quite sure there's a lot of utility in that (people aren't going to know if the snapshot is "right" or not, so will probably blindly accept changes). Really the "test" is sending the request to the server and seeing if it's happy.

The second one does some error handling logic also, but TBH I think we might revisit that as it's a bit weird.

@ghengeveld
Copy link
Member

I'm okay to leave the tests if there's no sensible strategy for it. Would be nice to have some E2E tests for this, eventually.

@ghengeveld
Copy link
Member

It seems the stories are failing because they use the manager-api which isn't available in the preview.

@tmeasday tmeasday merged commit 2ecb8d6 into main Sep 23, 2023
3 checks passed
@tmeasday tmeasday deleted the tom/ap-3549-onboarding-flow-for-new-users-to-create-a-project branch September 23, 2023 15:56
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.

2 participants