Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Add fontface definitions to Psammead #304

Merged
merged 21 commits into from
Feb 28, 2019
Merged

Add fontface definitions to Psammead #304

merged 21 commits into from
Feb 28, 2019

Conversation

bcmn
Copy link
Contributor

@bcmn bcmn commented Feb 22, 2019 •
edited
Loading

Resolves #285

Overall change: Investigation of how to handle Reith font faces in Psammead

Code changes:

  • Moves font family fallbacks to gel-foundations, as these are defined by GEL
  • Font-face declarations now live in psammead-styles, with the reasoning that these are our definitions of how browsers should use the files (see Sareh's comment below)
  • Only defines "ReithSans", etc. not "ReithSansLatin" -- the font files used contain Cyrillic characters as well, & we believe switching to use subsets would likely only undermine caching benefits across the main BBC services.
  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@dr3 dr3 assigned bcmn Feb 25, 2019
@bcmn bcmn changed the title First implementation of Psammead fontfaces Investigation of Psammead fontfaces Feb 26, 2019
sareh
sareh previously approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

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

👍 Looks great!

dr3
dr3 suggested changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

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

Just a small thing, otherwise looks really good :)

packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
ChrisBAshton
ChrisBAshton reviewed Feb 26, 2019
View reviewed changes
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Approach looks good, just some consistency tweaks and a question around the baseUrl for the fonts.

packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/fonts.js
src: url("${baseFontUrl}BBCReithSansCd_W_Rg.woff2") format("woff2"), url("${baseFontUrl}BBCReithSansCd_W_Rg.woff") format("woff");
font-display: optional;
}`;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a reith sans condensed italic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No -- only Regular & Bold, & apparently not any italic variants. I did my best to capture this in the new table.

packages/utilities/psammead-styles/src/fonts.js Show resolved Hide resolved
dr3
dr3 previously approved these changes Feb 26, 2019
View reviewed changes
@sareh sareh mentioned this pull request Feb 26, 2019
Closed
1 task
sareh
sareh previously approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

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

👍 Looks great. If you could update the PR from a 'draft' one to a standard PR, this could count as not just the investigation, but also the implementation and so close off both Issues.

@sareh
Copy link
Contributor

sareh commented Feb 26, 2019

  • As a caveat to my comment above - snapshot tests would need to be updated, prior to this being able to be a mergeable PR.

@bcmn bcmn dismissed stale reviews from sareh and dr3 via c75fcf4 February 27, 2019 09:47
@bcmn bcmn changed the title Investigation of Psammead fontfaces Add fontface definitions to Psammead Feb 27, 2019
@bcmn bcmn marked this pull request as ready for review February 27, 2019 09:58
@bcmn bcmn requested a review from a team as a code owner February 27, 2019 09:58
ChrisBAshton
ChrisBAshton previously approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

Looks good - thanks Ben

dr3
dr3 approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@dr3 dr3 left a comment

Choose a reason for hiding this comment

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

👍 LGTM, great work

sareh
sareh approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@sareh sareh left a comment

Choose a reason for hiding this comment

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

👍 Great work, Ben! I appreciate that you've updated the documentation to reflect use-cases, too.

@jamesbrumpton
Copy link
Contributor

👍

@bcmn bcmn merged commit 4cef741 into latest Feb 28, 2019
@bcmn bcmn deleted the inv-fontface branch February 28, 2019 12:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sareh sareh sareh approved these changes

@pjlee11 pjlee11 pjlee11 approved these changes

@dr3 dr3 dr3 approved these changes

@ChrisBAshton ChrisBAshton ChrisBAshton left review comments

Assignees

@bcmn bcmn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bcmn @sareh @jamesbrumpton @ChrisBAshton @pjlee11 @dr3