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: change fira source #1944

Merged
merged 8 commits into from
Apr 26, 2024
Merged

Conversation

wusteven815
Copy link
Contributor

@wusteven815 wusteven815 commented Apr 17, 2024

@wusteven815 wusteven815 self-assigned this Apr 17, 2024
@mofojed mofojed requested a review from dsmmcken April 18, 2024 13:41
@mofojed
Copy link
Member

mofojed commented Apr 18, 2024

@dsmmcken would these differences be expected/okay? Fonts appear okay to me, tabular numbering etc.

Actual:
typography-actual

Expected:
typography-expected

Diff:
typography-diff

From in the app on a table with numbers:
image

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

Missing Fira Mono, which is being substituted. The previous package included both Fira Sans and Mono, you'll have to add those in as well.

That's frustrating that the kerning tables must be different. Still looks acceptable, and has correct font-features-settings. 🤷

@dsmmcken
Copy link
Contributor

null

We are probably also missing an italic version in that weight.

@wusteven815 wusteven815 linked an issue Apr 23, 2024 that may be closed by this pull request
@wusteven815 wusteven815 marked this pull request as ready for review April 24, 2024 14:23
wusteven815 and others added 2 commits April 25, 2024 09:50
Co-authored-by: Don <dsmmcken@gmail.com>
Co-authored-by: Don <dsmmcken@gmail.com>
dsmmcken
dsmmcken previously approved these changes Apr 25, 2024
mofojed
mofojed previously approved these changes Apr 26, 2024
@wusteven815 wusteven815 dismissed stale reviews from mofojed and dsmmcken via a110e7e April 26, 2024 13:22
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Need to re-run npm install to update the package-lock.json now.

@wusteven815 wusteven815 merged commit 07e5a26 into deephaven:main Apr 26, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fira fonts require GitHub to install
3 participants