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

Fix erroneous CSS output after the last Style Dictionary upgrade #91

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

robintown
Copy link
Member

@robintown robintown commented Jul 12, 2024

This is based on #89

The font size rounding errors that we were seeing are fixed in version 1.1.0 of Tokens Studio Transforms. However, upgrading to this version also requires upgrading to a new major version of Style Dictionary at the same time. The upgrade isn't trivial as there are quite a few breaking changes, but it's good to get it over with.

Also fixes an issue introduced at the same time as the above, which broke the CSS output for spacing variables. See the second commit for details.

@sandhose
Copy link
Member

@robintown I squash-merged #89, so you probably want to change the base branch and rebase. By the way, if we merge normally instead of squashing, we wouldn't have to rebase like that, and PRs would automatically change base when the base branch gets merged into main…

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

I can't give my opinion about the style/naming changes since I barely know how to type ts, but the outputs seems sensible to me and the bug is fixed.

Comment on lines +25 to +26
public static let fontWeightBold = Font.Weight.bold
public static let fontWeightSemibold = Font.Weight.semibold
Copy link
Member

Choose a reason for hiding this comment

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

What happened here? 👀

Copy link
Member

Choose a reason for hiding this comment

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

That certainly looks more correct 😅 - we don't use any of the font tokens as they're based on the system ones so we have a manual mapping directly to those.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm really not sure, it looks like the values had been wrong ever since this file was first generated nearly a year ago. It's got to be a bug in Style Dictionary or Tokens Studio Transforms that was fixed in the new releases.

@pixlwave
Copy link
Member

pixlwave commented Jul 12, 2024

@robintown I squash-merged #89, so you probably want to change the base branch and rebase. By the way, if we merge normally instead of squashing, we wouldn't have to rebase like that, and PRs would automatically change base when the base branch gets merged into main…

I think the changing base part is because this repo doesn't auto-delete the branch after merging a PR. looks to enable it

Edit: Done, plus I've added the Update/Rebase Branch button and Auto-merge options.

The font size rounding errors that we were seeing are fixed in version 1.1.0 of Tokens Studio Transforms. However, upgrading to this version also requires upgrading to a new major version of Style Dictionary at the same time. The upgrade isn't trivial as there are quite a few breaking changes, but it's good to get it over with.
@robintown robintown changed the base branch from quenting/esm-cleanup to main July 12, 2024 14:59
@robintown robintown merged commit 5350cf2 into main Jul 12, 2024
@robintown robintown deleted the style-dictionary-4 branch July 12, 2024 15:00
@robintown
Copy link
Member Author

@robintown I squash-merged #89, so you probably want to change the base branch and rebase. By the way, if we merge normally instead of squashing, we wouldn't have to rebase like that, and PRs would automatically change base when the base branch gets merged into main…

I'm a fan of merging with merge commits as well! But our current workflows make that difficult sometimes…

  • GitHub will normally give you a "View changes" button when someone updates a PR with new commits since your last review.
  • But if the branch has been force-pushed, the button is no longer available and reviewing becomes more difficult, so at least in the Element Web sphere, reviewers ask PR authors to avoid force pushing.
  • That of course leads to messy commit histories all the time, so the EW team prefer squash merging as a default.

Personally I don't care a whole lot if it takes me a moment to find where the changes are, but I know that others can find it pretty annoying. It's unfortunate that GitHub still doesn't accommodate for force-push-heavy workflows :(

One thing we can do to improve this situation, is to at least give PR authors the option to merge with a merge commit in case they're making stacked changes. (turns the option on)

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.

4 participants