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

feat(fonts): use bc sans by default #197

Merged
merged 3 commits into from Mar 14, 2023
Merged

feat(fonts): use bc sans by default #197

merged 3 commits into from Mar 14, 2023

Conversation

roedoejet
Copy link
Collaborator

load from shadow dom as per ionic-team/stencil#2072

also removes requirement for parent applications to load google fonts separately

load from shadow dom as per ionic-team/stencil#2072

also removes requirement for parent applications to load google fonts separately
@roedoejet roedoejet requested a review from joanise March 8, 2023 18:22
Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Testing on my machine the Material-Icons fonts don't work anymore for this patch on the web component, for the play/rewind/stop etc buttons at the bottom of the component.

Comment on lines -13 to -17

<link
href="https://fonts.googleapis.com/css?family=Lato|Material+Icons|Material+Icons+Outlined"
rel="stylesheet"
/>
Copy link
Member

Choose a reason for hiding this comment

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

This change does not need to be reflected in the HTML that the Studio CLI produces, does it?
I think it doesn't, because if I understand this patch correctly, we bundle all our fonts, but I want to make sure.

Copy link
Member

Choose a reason for hiding this comment

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

I take it back, you've removed these lines from the sample files in test-data/ej-fra, so MINIMAL_INDEX_HTML_TEMPLATE in Studio/readalongs/text/util.py will need the same change when we make it depend on the web-component version that includes this PR.

Although I assume it's harmless to leave it in there when we don't use it, so it won't be urgent. But we should make an issue in the Studio repo so we remember to do this when it's time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, you're right - it's harmless to leave it in, but it's unnecessary. Let's remember to take it out. With this patch, the fonts shouldn't need to be declared in the parent (but it doesn't matter if they are, it'll ultimately just make for a redundant call to the API)

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

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

Works now, all good!

@joanise joanise merged commit ac2d354 into main Mar 14, 2023
1 check passed
@joanise joanise deleted the dev.fonts branch March 14, 2023 12:51
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.

None yet

2 participants