Open Bug 1531104 Opened 8 months ago Updated 3 months ago

Improve developer experience to avoid regressing visual components with many variations

Categories

(Firefox :: New Tab Page, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: Mardak, Unassigned)

References

(Blocks 1 open bug)

Details

As noted in bug 1531099, there have been CSS related regressions, but a larger picture is more generally making sure component changes don't break things and do what they should do in all variations. It was noted that dev-test-all is relatively comprehensive, but it's not a great experience in making sure "for all Lists, did changing X not break 1) sizes: 1 vs 2 vs 3 columns 2) images or not 3) numbered or not, etc." as they're scattered throughout a large page.

dmose would probably suggest Storybook

Iteration: --- → 68.1 - Mar 18 - 31
See Also: → 1532807

There are various things in this space, a web search will find them. As I recall, there was something :jlast liked even better than Storybook (am I thinking of Percy or Cypress or something else?).

Storybook is pretty good for this, seems to have the most mind-share, and now has the advantage that it's useful for non-React stuff too (eg pure CSS). Note that it will get us that coverage for manual development as well as for manual design feedback during development; if we want to do automated visual diffing, we'd probably need to do some work on top of it, or else to pay for a service that runs Storybook tests in CI (eg Chromatic, Percy, ...).

Flags: needinfo?(jlaster)

Er, s/pure CSS/pure HTML/.

Iteration: 68.1 - Mar 18 - 31 → ---

Storybook is great. I also think the percy integration for automated visual testing is really helpful.

Flags: needinfo?(jlaster)
Flags: needinfo?(dmose)

I've hacked together a prototype of using Storybook for a few DiscoveryStream components. An example of what this looks like is at https://dmose.github.io/activity-stream/.

The PR is at https://github.com/mozilla/activity-stream/pull/4923/files, and it's failing to pass the CI only because I didn't set it up with credentials that would allow it to push to master. Doing that cleanly would be something like

Left as an exercise for the next owner of this bug are:

  • securely making the credentials available to push this automatically during CI; https://wiki.mozilla.org/GitHub and https://wiki.mozilla.org/GitHub/Repository_Security are likely to be relevant here. Note, however, that we don't necessarily have to push the Storybook static build into the same repo, and the storybook-deployer allows it to be pushed to S3, which might end up making it easier.

  • making a link to the storybook for any built commit in the PR appear in the PR page.

Worth spinning out to other bugs:

  • making the (Travis) build turn red if visual regressions are found. https://storybook.js.org/docs/testing/automated-visual-testing/ points to lots of possible avenues here, including Percy, as mentioned above by Jason.

  • making Treeherder (Tier ?) turn red if visual regressions are found.

  • making Phabricator deploy Storybook on pushes to m-c

Flags: needinfo?(dmose)
Iteration: --- → 68.4 - Apr 29 - May 12
Priority: -- → P1
Iteration: 68.4 - Apr 29 - May 12 → ---
Priority: P1 → P3
No longer blocks: pocket-newtab-69
No longer blocks: pocket-newtab-68
See Also: → 1560235
Component: Activity Streams: Newtab → New Tab Page
Depends on: 1566531
No longer depends on: 1566531
See Also: → 1566531
Duplicate of this bug: 1531099
You need to log in before you can comment on or make changes to this bug.