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

[AP 3560] Build UI for build status #76

Merged
merged 28 commits into from
Sep 14, 2023

Conversation

weeksling
Copy link
Contributor

@weeksling weeksling commented Sep 6, 2023

This builds out most of the ui for the progress bar and build status, extending #56

Note this is not complete.

  • The onTaskCompleted callback does not work as intended, and it sets step to undefined until build.
  • The entire index side of this will change with recent changes so it needs to use whatever the latest is for passing in progress.
  • Steps in the cli do not accurately match the tasks in the cli. There are several steps in the design for the ui based on the actual cli output. These steps need to be added to the types possible on the BuildProgressPayload and in the actual cli.
  • This doesn't include the concept of stepsHistory yet. We need to add a useEffect that will store/update this value onCompletion of a step. This could also be passed as a prop.
📦 Published PR as canary version: 0.0.68--canary.76.d059219.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.68--canary.76.d059219.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.68--canary.76.d059219.0

@linear
Copy link

linear bot commented Sep 6, 2023

@weeksling weeksling changed the base branch from main to tom/ap-3560-build-ui-for-build-status September 6, 2023 04:04
@weeksling weeksling changed the base branch from tom/ap-3560-build-ui-for-build-status to main September 6, 2023 04:04
@tmeasday tmeasday mentioned this pull request Sep 6, 2023
@tmeasday tmeasday force-pushed the matt/ap-3560-ext-build-ui-for-build-status branch from cdb6e93 to 8e1d95c Compare September 6, 2023 10:49
@ghengeveld ghengeveld force-pushed the matt/ap-3560-ext-build-ui-for-build-status branch from 8e1d95c to 7cd0334 Compare September 8, 2023 22:37
@ghengeveld ghengeveld marked this pull request as ready for review September 11, 2023 14:07
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Looks great. A lot of smaller feedback. Probably my biggest concern is the "history" being storied in a leaf component's state, that seems fragile.

src/components/BuildProgressLabel.tsx Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/screens/VisualTests/BuildEyebrow.stories.ts Outdated Show resolved Hide resolved
src/screens/VisualTests/BuildProgress.tsx Outdated Show resolved Hide resolved
src/screens/VisualTests/BuildProgress.tsx Outdated Show resolved Hide resolved
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM, test seems like the right approach!

@ghengeveld ghengeveld merged commit 28ec539 into main Sep 14, 2023
3 checks passed
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.

3 participants