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 32619: clarify font-weight descriptor #32853

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Mar 27, 2024

Fixes #32619.
Fixes #10910

This is an attempt to improve the page on the font-weight descriptor. I think #32619 was fundamentally caused by the fact that this page isn't very clear about what the font-weight descriptor is, how it relates to the font-weight property, and how it is used. It includes stuff which is just wrong:

The values for the CSS descriptor is same as that of its corresponding font property.

...and stuff which makes it sound like this is the page for the property:

The font-weight property is described using any one of the values listed below.

I've rewritten the description and syntax sections, more or less, and added two live samples that I hope show how this can actually be used.

I'm no expert here so might have mangled things. I do think though that this documentation could be improved. I think we should probably have glossary entries for font, font face, typeface, font family. We should probably have a proper guide to using @font-face - this is the only thing I could find and it's very basic. And if these changes or something like them are good, we should probably do something similar with the other descriptors.

Oh, the live samples don't work, maybe because preview is broken? Sorry, you might need to test this locally.

@wbamberg wbamberg requested a review from a team as a code owner March 27, 2024 03:30
@wbamberg wbamberg requested review from estelle and removed request for a team March 27, 2024 03:30
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/m [PR only] 51-500 LoC changed labels Mar 27, 2024
Copy link
Contributor

github-actions bot commented Mar 27, 2024

Preview URLs

External URLs (2)

URL: /en-US/docs/Web/CSS/@font-face/font-weight
Title: font-weight

(comment last updated: 2024-10-29 20:43:17)

@wbamberg
Copy link
Collaborator Author

@bsmth , I know you care about the repo size, and this PR adds some sample fonts to the repo, and one of them at least is quite big (127K). There was some chat before about having a separate repo for assets that live samples could refer to. That still feels like the proper solution. Do you know what the status of this is? I see https://github.com/mdn/shared-assets exists, can it be referenced from live samples in mdn/content?

@bsmth
Copy link
Member

bsmth commented Apr 2, 2024

@bsmth , I know you care about the repo size, and this PR adds some sample fonts to the repo, and one of them at least is quite big (127K). There was some chat before about having a separate repo for assets that live samples could refer to. That still feels like the proper solution.

Yes, that was the idea, indeed.

Do you know what the status of this is? I see https://github.com/mdn/shared-assets exists, can it be referenced from live samples in mdn/content?

This can't be used yet, my hope was that it would be added to a GCP bucket that we could reference like demo-assets.developer.mozilla.org or something along those lines. I'm going to check again what the status is shortly and I'll come back to this PR with some info.

@bsmth
Copy link
Member

bsmth commented Apr 2, 2024

Hey @wbamberg - quick update to get something going in the meantime: I've enabled gh-pages for the assets repo in mdn/shared-assets#8

This allows you to use the fonts from that repo and add your own if you need different ones. The examples cleanup work we'll be doing later in the year will likely consider this assets repo and have more robust deployment, but that should unblock you for now (although we may need a CSP entry for mdn.github.io/shared-assets)

estelle

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Just a reminder you can use any of these instead of landing the assets in the content repo:

https://github.com/mdn/shared-assets/tree/main/fonts

i.e., https://mdn.github.io/shared-assets/fonts/FiraSans-Light.woff2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, now we're using Fira Sans and League Mono!

@Josh-Cena
Copy link
Member

I am linking #10910 to this PR. Would be helpful if you could see if there's anything else to be done about the issue, but I think it already suffices as a fix.

@Josh-Cena Josh-Cena added the awaiting response Awaiting for author to address review/feedback label Aug 27, 2024
@Josh-Cena
Copy link
Member

@wbamberg @estelle Is it possible that either of you could finish this PR? This is currently one of the oldest ones.

@wbamberg
Copy link
Collaborator Author

Sorry @Josh-Cena . I will attempt to remember what's going on here.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

I am breaking things

files/en-us/web/css/@font-face/font-weight/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@font-face/font-weight/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@font-face/font-weight/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@font-face/font-weight/index.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Awaiting for author to address review/feedback Content:CSS Cascading Style Sheets docs size/m [PR only] 51-500 LoC changed
Projects
None yet
4 participants