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

Use new font face and font family in psammead packages #318

Merged
merged 9 commits into from
Mar 6, 2019
Merged

Use new font face and font family in psammead packages #318

merged 9 commits into from
Mar 6, 2019

Conversation

dr3
Copy link
Contributor

@dr3 dr3 commented Mar 4, 2019 •
edited
Loading

Resolves #312

Overall change: Updates to use new font-family

Code changes:

  • Updates psammead to use new font family
  • Updates storybook to use new font face

Storybook visual regression:

  • psammead-caption - Looks same
  • psammead-headings - Looks same
  • psammead-paragraph - Looks same
  • psammead-copyright - Looks taller due to em -> rem change
  • psammead-sitewide-links - Looks bolder
    Was previously using font-weight: 700; font-family: ReithSansNewsRegular, Helvetica, Arial, sans-serif; However due to this change it has gone to using F_REITH_SANS_BOLD which looks bolder. Will talk with UX about possibly not using bold
old new no bold
image image image
  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@dr3 dr3 added the v1.0 label Mar 4, 2019
@dr3 dr3 self-assigned this Mar 4, 2019
@dr3 dr3 requested a review from a team as a code owner March 4, 2019 17:36
@dr3 dr3 changed the title Use font face Use new font face and font family in psammead packages Mar 4, 2019
@dr3 dr3 added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Mar 5, 2019
@dr3 dr3 removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Mar 5, 2019
@pjlee11
Copy link
Contributor

pjlee11 commented Mar 6, 2019

Please ensure this goes through a UX review

pjlee11
pjlee11 reviewed Mar 6, 2019
View reviewed changes
packages/components/psammead-headings/src/__snapshots__/index.test.jsx.snap
margin: 0;
padding: 2rem 0 1rem 0;
font-size: 1.75em;
font-weight: 500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a font-weight?

Copy link
Contributor

Choose a reason for hiding this comment

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

Spoke to Drew IRL and this matches the font-weight we have set on the font meaning it would switch to use the bold source file. Nominating @dr3 to create an issue to put these values into a package to be re-usable meaning it's less likely someone will use "faux bold" by mistake EG: font-weight: 450;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, as weve gone from Medium

Copy link
Contributor

@pjlee11 pjlee11 Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Drew*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Donneee #327

@dr3
Copy link
Contributor Author

dr3 commented Mar 6, 2019

Have discussed with @nataliesmart about the visual changes for the copyright and sitewidelinks, looks good

pjlee11
pjlee11 previously approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

@sareh
Copy link
Contributor

sareh commented Mar 6, 2019

@dr3 Looks great locally. You just need to resolve the package version conflict issues & I'll be happy to approve this.

sareh
sareh approved these changes Mar 6, 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
Reith Gif

@dr3
Copy link
Contributor Author

dr3 commented Mar 6, 2019

This has been reviewed by Natalie this morning :) Moving into test

@jamesbrumpton
Copy link
Contributor

👍

@dr3 dr3 merged commit 5ec3c47 into latest Mar 6, 2019
@dr3 dr3 deleted the UseFontFace branch March 6, 2019 15:37
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

Assignees

@dr3 dr3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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