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

Create index.css for open-sans-condensed #99

Closed
wants to merge 1 commit into from
Closed

Create index.css for open-sans-condensed #99

wants to merge 1 commit into from

Conversation

ed-wright
Copy link

Created an index.css file for open-sans-condensed so that the file can be used out of the box with a default font weight.

Created an index.css file for open-sans-condensed so that the file can be used out of the box with a default font weight.
@ed-wright ed-wright changed the title Create index.css Create index.css for open-sans-condensed Dec 1, 2020
@ed-wright
Copy link
Author

ed-wright commented Dec 1, 2020

Addresses issue #68, weight 300 was picked as the default. I realise this doesnt update the documentation but I am willing to help solve that with some consensus.

I wrote a python script to identify packages that are missing the index.css file.

import os
from pathlib import Path
directory = "packages"

for font_directory in next(os.walk(directory))[1]:
    filename = str(Path(".", directory, font_directory, "index.css"))
    if not os.path.isfile(filename):
        print(font_directory)

Which gave the following output:

  • buda
  • coda-caption
  • open-sans-condensed
  • sunflower
  • unifrakturcook

@ayuhito
Copy link
Member

ayuhito commented Dec 2, 2020

I appreciate the pull request, but I would prefer a programmatic approach that even I'm willing to patch myself if there was some input on whether we would prefer weight 300 or 500 in the case both were present discussed in #68.

Quite frankly, that doesn't matter. I'm fine with choosing the lower weight option if present since one has to be chosen and there's no real reason to choose either.

The reason why it has to be programmatically implemented is that index.css would be removed upon the next time Google pushes an update to any of the fonts that do not have index.css. I don't necessarily feel comfortable allowing this pull request through when I don't know when Google could push an update for one of these fonts, where the repository will automatically rebuild the package which won't include the old index.css files. That's a breaking update that could ruin some production sites that might auto-merge patch updates.

It's relatively easy to apply this fix programmatically in packager-v2.js and the documentation is also another small adjustment to templates.js.

But also, will these changes be necessary for #89? If that doesn't go through, I'd be more than open to make the relevant changes to fix this.

@ed-wright
Copy link
Author

ed-wright commented Dec 2, 2020

@DecliningLotus, I completely agree.

Would you consider the idea of changing the index.css? Instead of only containing the 400 font replacing it with all available font-faces for that particular font. This would avoid any breaking change, and would standardise the index.css across all fonts. This obviously would increase import size but users looking to decrease package size will look through the docs and implement one of the subset css files. This would allow users that are new to the project to get started faster and users with size constraints to use the project to its full potential. IMHO (ease of use through standardisation) >> (small code size)

I am happy to attempt to implement this change.

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