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

Added offline font. #1034

Merged
merged 3 commits into from
Dec 27, 2023
Merged

Added offline font. #1034

merged 3 commits into from
Dec 27, 2023

Conversation

chase-west
Copy link
Contributor

@chase-west chase-west commented Nov 3, 2023

Related issue

Closes #1031

Context / Background

  • Added offline font for Montserrat.

What change is being introduced by this PR?

  • I added a offline font family.
  • I changed the source for the font family.
  • It allows the font to change when offline.

How will this be tested?

Check if font works when offline.

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (26f5d4c) 79.37% compared to head (4d0a82b) 79.40%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1034      +/-   ##
==========================================
+ Coverage   79.37%   79.40%   +0.03%     
==========================================
  Files          21       21              
  Lines        1275     1277       +2     
  Branches      189      189              
==========================================
+ Hits         1012     1014       +2     
  Misses        263      263              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@araujoarthur0 araujoarthur0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It seems we only require two of the fonts from the family.
Could we commit only these two then?
And could you document in the .css file where you got it from, in case we need the other fonts in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file doesn't need to be committed, nothing changed in the package.

@chase-west
Copy link
Contributor Author

Thanks for the PR!

It seems we only require two of the fonts from the family.

Could we commit only these two then?

And could you document in the .css file where you got it from, in case we need the other fonts in the future?

Sorry it appears I didn't explain well. It only uses those 2 fonts when offline not the entire family. And yes I can document it.

@araujoarthur0
Copy link
Collaborator

In that case, let's commit only those two we need and not the whole family to have a smaller footprint in the repo.

@chase-west
Copy link
Contributor Author

In that case, let's commit only those two we need and not the whole family to have a smaller footprint in the repo.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this one required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to change styles

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this one required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to change styles

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

@chase-west can you please undo any changes on the package-lock.json? It should have not been necessary to touch it for the PR

@tupaschoal
Copy link
Collaborator

@chase-west ?

@chase-west
Copy link
Contributor Author

@chase-west can you please undo any changes on the package-lock.json? It should have not been necessary to touch it for the PR

I honestly have no idea how to do that I've tried

@araujoarthur0
Copy link
Collaborator

https://devconnected.com/how-to-remove-files-from-git-commit/
For more instructions on removing a file from a commit

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

@araujoarthur0 I have reverted the changes to package-lock.json, but I haven't tested the PR, can you check, please?

@araujoarthur0
Copy link
Collaborator

I've also changed the PR to use the variable weight font instead.
From my testing, this new font increased the font size and weight in some places in the tool, even though the actual font size and weight numbers are the same. My guess is because the format changed from woff to tty, but I'm not knowledgeable enough in this area to comment further. I've adjusted the sizes manually by comparing against the previous screen size occupied by the text.
I've only done this in Windows, though.

@thamara, could you check in Mac if the text sizes look the same before and after this PR?
For example the preferences window looked 1.5x bigger for me before adjusting it.

@thamara
Copy link
Collaborator

thamara commented Dec 27, 2023

I've checked this PR on Mac and everything seems fine.

With this PR:
pr

With the main branch:
main

@thamara
Copy link
Collaborator

thamara commented Dec 27, 2023

\changelog-update
Message: Fix [#1034]: Preserving TTL font when offline
User: chase-west

@thamara thamara merged commit 93b7fed into TTLApp:main Dec 27, 2023
9 checks passed
@thamara
Copy link
Collaborator

thamara commented Dec 27, 2023

Thank you all.

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.

App has no font when offline
4 participants