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

🐛 commit svg tester diffs to different branches #3211

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Feb 19, 2024

Problem

  • Both SVG tester actions commit to the same branch, but force-push their changes, which means that they overwrite changes on remote
  • As a result, the last action that pushes wins and overwrites the commit of the other action

Solution

  • We could attempt to fix this, but I never felt strongly about having both actions commit to the same branch
  • It's easier and less error-prone to just have both actions commit to different branches
    • If on branch <my-branch>, then the first action now commits to a branch of the same name, and the other action commits to <my-branch>__all-views

Tests

The follow-up PR makes the SVG tester actions fail on purpose. Note that both actions commit to different branches:

SVG tester:

Screenshot 2024-02-19 at 13 59 55 Screenshot 2024-02-19 at 14 00 24

SVG tester (all views):

Screenshot 2024-02-19 at 14 00 35 Screenshot 2024-02-19 at 14 00 49

@sophiamersmann
Copy link
Member Author

sophiamersmann commented Feb 19, 2024

@sophiamersmann sophiamersmann changed the title 🐛 (svg-tester) run svg tester actions from different branches 🐛 commit svg tester diffs to different branches Feb 19, 2024
@sophiamersmann sophiamersmann marked this pull request as ready for review February 19, 2024 13:05
@sophiamersmann sophiamersmann force-pushed the fix-svg-tester-actions branch 2 times, most recently from 8b3e0f0 to b545bec Compare February 23, 2024 08:26
Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Looks like a good approach!

@sophiamersmann sophiamersmann merged commit f8f12b1 into master Feb 23, 2024
26 checks passed
@sophiamersmann sophiamersmann deleted the fix-svg-tester-actions branch February 23, 2024 13:14
sophiamersmann added a commit that referenced this pull request Mar 1, 2024
> [!IMPORTANT]
> Wait for #3211 to be merged.

### Current state

- When exporting to static charts, we adjust the font size to fit the chart title into a single line if possible
    - The font size is decreased by not more than 20% using 0.5px steps
- But we don't currently do this on the web

### Changes

- We re-introduce this technique for charts rendered in an interactive environment (we had removed this for interactive charts in the redesign)
- I think reducing the font size by 20% is quite a lot, so I opted for 15% instead (the old grapher also used 85% of the original font size as a threshold)
- With 85%, the font size gets at most reduced to...
    - for narrow charts: 18px * 0.85 = 15.3px (subtitle font size = 12px)
    - for medium charts: 20px * 0.85 = 17px (subtitle font size = 13px)
    - for larger charts: 24px * 0.85 = 20.4px (subtitle font size = 14px)

The SVG tester complains because we used to decrease by no more than 20% but now use 15%. Some charts look "worse" because of that, but these thresholds are arbitrary, and we have to draw the line somewhere...
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