Problem/Motivation

So far we can find several components using both syntax: font-weight: normal and font-weight: 400, or font-weight: bold and font-weight: 700 in the Umami theme.
font-weight: normal is the same asfont-weight: 400 and it's not a matter of browser compatibility since all accept both syntax.

Proposed resolution

Decide if we should use font-weight: normal/font-weight: 400 and font-weight: bold/font-weight: 700and implement it.

Remaining tasks

  • Decide which syntax we should use.
  • Review the current code to be sure we use the chosen syntax everywhere.
CommentFileSizeAuthor
#15 numeric-values-2950158-15.patch5.71 KBVidushi Mehta
#9 font_weight_umami-2950158-9.patch7.64 KBShiva Srikanth T
#6 font-weight-on-Umami-theme-2950158-6.patch5.2 KBankitjain28may
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ckrina created an issue. See original summary.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

"font-weight: normal" is much more prevalent in core than font-weight: 400':

eli@rascal-2 [09:27:08] [~/code/drupal8/drupal/core] [8.6.x *]
-> % grep -r 'font-weight: normal' * | grep -v umami | wc -l
      50
eli@rascal-2 [09:28:34] [~/code/drupal8/drupal/core] [8.6.x *]
-> % grep -r 'font-weight: 400' * | grep -v umami | wc -l
       3

so I'd suggest going with "font-weight: normal" all other things being equal.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina as a volunteer commented
Issue summary: View changes

Sorry, I wasn't specific enough: we should decide between:
- font-weight: normal or font-weight: 400
- font-weight: bold or font-weight: 700

In general I prefer using the numbered weight because it clearly indicates which one are you using (IE font-weight: normal can be faux font-weight: 200).

But since this project is clearly delimited, I'd be OK with font-weight: normal and font-weight: bold (even I'd prefer the numbered one ;) ).

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina as a volunteer commented
Issue tags: +dcruhr18
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina as a volunteer commented
Issue tags: +Novice
ankitjain28may’s picture

ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented
FileSize
font-weight-on-Umami-theme-2950158-6.patch5.2 KB

Added the patch as per the above requirements.

ankitjain28may’s picture

ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented
Status: Active » Needs review
Shiva Srikanth T’s picture

Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commented
Status: Needs review » Needs work

@ankitjain28may issue is related to Component Umami demo but you have created a patch for core misc, module and theme css files, patch should be created for umami theme.

Shiva Srikanth T’s picture

Shiva Srikanth T CreditAttribution: Shiva Srikanth T as a volunteer commented
Status: Needs work » Needs review
FileSize
font_weight_umami-2950158-9.patch7.64 KB

Uploaded new patch.

The last submitted patch, 6: font-weight-on-Umami-theme-2950158-6.patch, failed testing. View results

The last submitted patch, 6: font-weight-on-Umami-theme-2950158-6.patch, failed testing. View results

The last submitted patch, 6: font-weight-on-Umami-theme-2950158-6.patch, failed testing. View results

Vidushi Mehta’s picture

Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commented
Status: Needs review » Reviewed & tested by the community

#9 has changed the font-weight on all files which was decided according to this issue in the comments thread. Marking as RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

Umami uses webfonts which we should take that into account to the consideration since most of Drupal core haven't been built webfonts in mind. As pointed out by @ckrina on #3, the mapping of weight names to numeric values might be different between browsers. Since we are planning to only import specific weights of the webfont in #2943657: Embedding only needed weights for Open Sans and Scope One we should use the numeric values to ensure that webfont for the weight is included.

Vidushi Mehta’s picture

Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commented
Status: Needs work » Needs review
FileSize
numeric-values-2950158-15.patch5.71 KB

Added a patch which has numeric values on all the files.

markconroy’s picture

markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented

I agree with @lauriii in #14.

It ensures we have a consistent experience across browsers and is more future-proof.

markconroy’s picture

markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented
Status: Needs review » Reviewed & tested by the community

Thanks @Vidushi Mehta

The patch from #15 applies cleanly and replaces all instances of font-weight: <word>.

This will be even better when #2943657: Embedding only needed weights for Open Sans and Scope One lands. Thanks for you work.

  • lauriii committed cdbb68b on 8.6.x 
    Issue #2950158 by Vidushi Mehta, ankitjain28may, Shiva Srikanth T,...

  • lauriii committed e3e7141 on 8.5.x 
    Issue #2950158 by Vidushi Mehta, ankitjain28may, Shiva Srikanth T,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thank you for the review and documenting the steps you took on reviewing the patch!

Looks good! I also confirmed that the patch still changes all the instances of font-weight to use numeric values.

Committed cdbb68b and pushed to 8.6.x. Also cherry-picked e3e7141 and pushed to 8.5.x. 🚀Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.