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

Show project picker when no project id is in main.js #10

Merged

Conversation

weeksling
Copy link
Contributor

@weeksling weeksling commented Jun 28, 2023

Screen.Recording.2023-07-05.at.12.47.47.PM.mov

Adds a parameter for the projectId and lets the user select between projects they are associated with. Includes serverChannel method to mutate the storybook configuration when setting the projectId & projectToken.

Not working yet:

  • useParameters pulls values from preview.ts, but not from addon options. Need to find a way to use option parameters from main.ts instead.

Pull request base is test-screens to make the changes here easier to diff. Should be changed to main prior to merging.
This depends on related Chromatic api PR https://github.com/chromaui/chromatic/pull/7460

📦 Published PR as canary version: 0.0.4-canary.10.1175a32.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.4-canary.10.1175a32.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.4-canary.10.1175a32.0

@linear
Copy link

linear bot commented Jun 28, 2023

AP-3260 Project Picker with Chromatic api and token

After the user has logged into Chromatic, the onboarding asks them to select a project to connect to. Using the authentication token, and the public API, implement a project picker.

Questions/Unknowns:

  • How to persist this selected project?
    • Localstorage, or manual config in main.js
    • Can we get access to this information with the projectToken somehow?
  • Can we use the Chromatic design system for the UI? Would this increase the addon's size too much?

@weeksling weeksling requested a review from ghengeveld July 5, 2023 16:59
src/utils/useProjectId.ts Outdated Show resolved Hide resolved
.storybook/main.ts Outdated Show resolved Hide resolved
@weeksling weeksling force-pushed the matt/ap-3260-project-picker-with-chromatic-api-and-token branch from 085edc7 to 84e978f Compare July 11, 2023 19:41
@thafryer thafryer changed the base branch from test-screens to main July 12, 2023 12:38
@tmeasday
Copy link
Member

tmeasday commented Jul 13, 2023

@ndelangen can you take a look, especially at the preset code-- @ghengeveld can you review the UI code.

I have it working doing the following:

  1. Reading project id + token from main.js
  2. Running a git command on startup
  3. Setting all of the above in process.env (sort of similar to your idea of automatically setting addon options in the build).
  4. Writing to main.js when you select a project.

What's missing/broken right now is:

  1. Because we use env to set initial values, if you select a project and refresh the browser, you'll lose track of the selected project.
  • We could store the selected project in local storage to avoid this
  • Or we could change it to request the selected project + git info via an event on bootup as originally planned. WDYT @ndelangen?
  1. Similarly if you change branch or commit but there's initial value built into the SB it'll go out of date if you refresh (although in that case I expect to poll for values). So maybe events is better?

3. The branch behaviour is weird in other ways but I'm not looking to fix that in this PR.

If y'all agree we could just say we are OK with 1+2 as limitations right now and fix it in a later ticket. I'm leaning that way.

@tmeasday tmeasday requested a review from ndelangen July 13, 2023 06:08
@tmeasday tmeasday marked this pull request as ready for review July 13, 2023 06:14
@ndelangen
Copy link
Member

Or we could change it to request the selected project + git info via an event on bootup as originally planned. WDYT

I feel like that's what I've been saying for a while, that will yield the best result, no weird behavior for anyone.
It won't support static build, but the addon won't support static storybook anyway.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member

I feel like that's what I've been saying for a while, that will yield the best result, no weird behavior for anyone.
It won't support static build, but the addon won't support static storybook anyway.

I guess so. But it seems overly complex to have 2 events just to request some data. Is that the pattern we want to lay down for client initiated server->client communication?

Other options:

  • HTTP requests rather than server channel (like /index.json) for data requests
  • Some API that abstracts away how it is implemented (which I think you also suggested).

My inclination is to take it a bit further like this just to gather information and then thrash it out very soon.

src/index.ts Outdated Show resolved Hide resolved
ghengeveld and others added 4 commits July 14, 2023 10:05
src/Panel.tsx Outdated Show resolved Hide resolved
@ghengeveld
Copy link
Member

I added slug to the lastBuild field args. This requires an update on the API side.

@tmeasday tmeasday changed the title Matt/ap 3260 project picker with chromatic api and token Show project picker when no project id is in main.js Jul 15, 2023
@ghengeveld ghengeveld merged commit 1175a32 into main Jul 15, 2023
@ghengeveld ghengeveld deleted the matt/ap-3260-project-picker-with-chromatic-api-and-token branch July 15, 2023 08:10
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.

4 participants