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

feat: re-introduce comment-based trigger for Chromatic #1468

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Sep 4, 2023

Problem

Chromatic snapshots are expensive and we should have a way to get a snapshot only when we really need one.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Features:

  • A second attempt of triggering Chromatic snapshots via a PR comment. Here's what changed from previously:
    • A new branchName configuration has been added. This is needed to let Chromatic be aware of where the snapshot is coming from. Without this configuration, Chromatic will pull from the GITHUB_REF environment variable, which is always develop when on the issue_comment workflow. This causes Chromatic to constantly rebuild the develop branch and causing our baselines to always reset.
    • A new autoAcceptChanges configuration has been added. This is needed so that the baseline (which should, in theory, be the develop branch) is up-to-date, as Chromatic treats the latest accepted ancestor build as the baseline to compare pull request changes against. This is done together with the push event to the develop branch, which will generate a new Chromatic snapshot that Chromatic will automatically mark as accepted.

Notes:

You tell me that you can only test after this is merged, how am I going to review this?

I hear your concerns. This was what I did to test my hypothesis:

  • Visit the test repository that I created to simulate Chromatic builds and the Chromatic build history.
  • Step through the changes made (using the test-1, test-2, test-3, test-4 and test-5 branches, along with their associated PRs).
  • Step through the Chromatic builds made (including the ones for develop).
  • Analyse the ancestor build(s) that Chromatic has chosen. The builds chosen should make sense (baseline should be develop as much as possible, or the parent commit of the same branch).
  • Finally, check that the syntax of the workflow file is correct.

If you are interested, the full documentation of how Chromatic calculates the ancestor builds is available.

Tests

Developer experience changes, no user-facing changes. Testing can unfortunately only be done once it is merged.

Deploy Notes

None

@dcshzj dcshzj requested a review from a team September 4, 2023 02:29
@dcshzj dcshzj temporarily deployed to staging September 4, 2023 02:41 — with GitHub Actions Inactive
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for the comprehensive write up!

@dcshzj
Copy link
Contributor Author

dcshzj commented Sep 6, 2023

Going to merge this now, we are not urgently using Chromatic so it would give me sufficient time to fine-tune this.

@dcshzj dcshzj merged commit 8953a6f into develop Sep 6, 2023
10 checks passed
@mergify mergify bot deleted the feat/chromatic-trigger branch September 6, 2023 03:21
@alexanderleegs alexanderleegs mentioned this pull request Sep 7, 2023
24 tasks
alexanderleegs added a commit that referenced this pull request Sep 7, 2023
* feat: re-introduce comment-based trigger for Chromatic (#1468)

* chore(contactus): styling fixes based on design feedback (#1470)

* chore(contactus): styling fixes based on design feedback

* fix: use subhead-1 styling instead

* chore: remove unused import

* Chore/fix edit nav bar styles (#1466)

* fix: nav menu template styles

* fix: add spacing for legacy nav section

* chore: remove unused attributes

* chore: move flex into className

* Chore/fix title text (#1472)

* chore: fix title text

* chore: fix other instances of spacing

* fix(edithomepage): spread properly (#1474)

Co-authored-by: seaerchin <[email protected]>

* Fix/edit nav nits (#1476)

* fix: reduce bottom padding of sidebar

* fix: text styling

* fix: padding on add section button

* chore: remove periods from validators

* chore: change placeholder text

* fix: update default values of new sections

* fix: copy changes

* fix(homepage): various styling fixes  (#1477)

* fix(edithomepage): fixed spacing between card/button

* fix(editable): fixed styling

* fix(editable): updated padding of editable accordion button

* fix(hero-highlight): fixed copy

* fix(editable): update padding

* fix(hero-higihlihgt-section): update wording

* refactor(formmediainput): add width

* fix(editable): fixed border radius on hover

---------

Co-authored-by: seaerchin <[email protected]>

* feat(editable): introduce new nested card variant (#1478)

* feat(icons): introduce new vertical draggable icon

* feat(editable): introduce nested version of accordion

* fix(contactus): update location card to use nested

* fix: border radius of error divider

* 0.42.0

---------

Co-authored-by: seaerchin <[email protected]>
Co-authored-by: Hsu Zhong Jun <[email protected]>
Co-authored-by: seaerchin <[email protected]>
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