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.

Sign up for GitHub

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

Jump to bottom

ci: adds basic visual regression testing for icons #16408

Merged
merged 11 commits into from
Sep 4, 2024
Merged

ci: adds basic visual regression testing for icons #16408

merged 11 commits into from
Sep 4, 2024

Conversation

zomars
Copy link
Member

@zomars zomars commented Aug 30, 2024 •
edited
Loading

What does this PR do?

  • Allows Icon fills to be overridden
  • Fixes verified badge on booking pages
  • Adds icons page to showcase all icons
  • Adds visual regression testing to prevent visual bugs
  • Adds missing icons

image
image
image

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • yarn e2e -g "Icons render properly"

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

@vercel Vercel
Copy link

vercel bot commented Aug 30, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Aug 31, 2024 1:01am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Aug 31, 2024 1:01am

@graphite-app graphite-app bot requested a review from a team August 30, 2024 01:11
@keithwillcode keithwillcode added core area: core, team members only foundation labels Aug 30, 2024
@dosubot dosubot bot added automated-tests area: unit tests, e2e tests, playwright ci area: CI, DX, pipeline, github actions labels Aug 30, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented Aug 30, 2024

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (08/30/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@zomars zomars marked this pull request as draft August 30, 2024 01:19
zomars
zomars commented Aug 30, 2024
View reviewed changes
apps/web/public/icons/sprite.svg
@@ -2,77 +2,77 @@
<!-- This file is generated by npm run build:icons -->
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="0" height="0">
<defs>
<symbol class="lucide lucide-activity" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" id="activity">
<symbol class="lucide lucide-activity" viewBox="0 0 24 24" fill="inherit" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" id="activity">
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows using tailwind "fill-*" classes to work properly

zomars
zomars commented Aug 30, 2024
View reviewed changes
packages/ui/components/icon/icon-list.mjs Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted alphabetically, removed duplicated ones.

zomars
zomars commented Aug 30, 2024
View reviewed changes
packages/ui/scripts/build-icons.mjs
@@ -89,6 +89,7 @@ async function generateSvgSprite({ files, inputDir, outputPath }) {

svg.tagName = "symbol";
svg.setAttribute("id", iconName(file));
svg.setAttribute("fill", "inherit");
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main fix

@zomars zomars marked this pull request as ready for review August 30, 2024 02:07
@dosubot dosubot bot added the ui area: UI, frontend, button, form, input label Aug 30, 2024
@zomars zomars requested a review from a team August 30, 2024 02:12
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 30, 2024 •
edited
Loading

E2E results are ready!

@zomars zomars enabled auto-merge (squash) August 31, 2024 01:16
@zomars zomars added the Medium priority Created by Linear-GitHub Sync label Sep 2, 2024 — with Graphite App
@keithwillcode keithwillcode added this to the v4.5 milestone Sep 3, 2024
@zomars zomars merged commit 36e79bf into main Sep 4, 2024
80 of 82 checks passed
@zomars zomars deleted the test/icon-regression branch September 4, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbjORbj hbjORbj hbjORbj approved these changes

Assignees
No one assigned
Labels
automated-tests area: unit tests, e2e tests, playwright ci area: CI, DX, pipeline, github actions core area: core, team members only foundation Medium priority Created by Linear-GitHub Sync ready-for-e2e ui area: UI, frontend, button, form, input
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.
3 participants
@zomars @hbjORbj @keithwillcode