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

Refactor BuildInfo -> StoryInfo #36

Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Aug 17, 2023

This was the information it was intended to convey, I guess.

  • Refactored test summary information so we can use it in multiple places without passing heaps of data around
  • Refactored story data creation in StoryInfo and SnapshotComparison (we might do the same in VisualTests)
  • Fixed up the behaviour to in particular deal with the isStarting state and broken builds that never started.

It feels better but still possibly a bit over-complicated.

📦 Published PR as canary version: 0.0.25--canary.36.05c9960.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.25--canary.36.05c9960.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.25--canary.36.05c9960.0

@linear
Copy link

linear bot commented Aug 17, 2023

AP-3450 BuildInfo component errors if Build has errors

It's possible for a build to be in status: BROKEN but not have tests or startedAt if extract fails.

I'm not sure if this is a schema failure on the Chromatic side? Should we have used FAILED for these? Not sure: https://github.com/chromaui/chromatic/blob/0af20121e99d5cf1d261e485560c330f3c34be6d/services/index/model/Build.js#L1088-L1092

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.

Nice cleanup 👍

Copy link
Contributor

@weeksling weeksling left a comment

Choose a reason for hiding this comment

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

Neat! I might need to update my pr to use this

@tmeasday
Copy link
Member Author

tmeasday commented Aug 18, 2023

@MichaelArestad can you take a look at the UI Review please?


@ghengeveld let's discuss the implementation of "click the button and see the old UI for a sec".

Currently it's difficult to achieve it reliably because the isStarted field doesn't actually allow us distinguish between:

a) the current build is starting and is reached the announced state OR

b) we don't have a new build yet, so build is still the previous build.

This is due to isStarted not unsetting until the new build reaches PREPARED.

So one solution might be to clear isStarted as soon as the CLI knows the build id (or add a second var to achieve the same purpose). But we might not actually have the new build yet!

Another option would be to store runningBuildId and set it to null when starting a new build, then use that to tell if build is about to be replaced or not. We could have a line like:

buildIsCurrent = build?.id === runningBuildId

And then in the StoryInfo component we hide the build if isStarting && buildIsCurrent but show the build if isStarting && !buildIsCurrent. WDYT?

@ghengeveld
Copy link
Member

ghengeveld commented Aug 18, 2023

@ghengeveld let's discuss the implementation of "click the button and see the old UI for a sec".

Let's start with the desired behavior. There's two parts: the buttons' spinner/disabled state and the Panel itself.

When we have a build (CI or earlier local build, doesn't matter) and we run a new one, we want the old build to remain visible in the Panel until the new build starts to gather useful results (i.e. it's snapshotting). Until then it's would be convenient to be able to see the previous build. Perhaps we should even leave the old snapshot until the new one is available (i.e. the tests for that particular story are done). Nobody really wants to be looking at a spinner while they could be looking at something useful (even if old). We do have to take into account that the old snapshots cannot be accepted anymore once the new build is announced and of course it needs to be apparent that new snapshots are on the way.

Meanwhile, I think it should not be possible to start another build until the current one is at least fully handed over to the cloud (i.e. it's been published), so as not to interrupt the CLI while it's still working on announcing/uploading a build. I think the button's disabled state should be decoupled from its 'loading' state, because the loading state should probably be enabled until the build is completely finished. A simple spinner isn't really going to cut it either, we should probably have some sort of progress indicator as well as hover tooltip that explains the current state of the build (and if there is no build running; when the last build finished).

To make this all possible, we need the following:

  • ID and status of the currently running build (or perhaps the current step of the CLI)
  • Progress of the currently running build (finished test count ÷ total test count)
  • All tests for the currently selected story from both the old and new build

Currently we stop fetching/polling the old build as soon as we have a new (announced) build ID. I think this is fine because once that happens, the old build will be locked and won't change anymore, but we do have to keep the old build data around until the new build is finished. This is extra tricky if we decide to show new snapshots as soon as they are available, even if the build is still testing other stories, because UX wise this may be confusing (it needs to be very clear what information is old).

All in all I think this whole thing is mostly a UX question right now and we should defer making major changes until we know where we're going.

@tmeasday
Copy link
Member Author

Going to approve the last two snapshots as we are still discussing how to do deal with these states and we'll do another pass over this. @MichaelArestad feel free to UI review, but I'll merge this PR without it because I think we'll be doing another pass over this component very soon.

@tmeasday tmeasday merged commit 75a037a into main Aug 21, 2023
3 checks passed
@tmeasday tmeasday deleted the tom/ap-3450-buildinfo-component-errors-if-build-has-errors branch August 21, 2023 01:41
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