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

Add global styles to psammead-styles #2021

Merged
merged 20 commits into from
Sep 12, 2019
Merged

Conversation

EinsteinNjoroge
Copy link
Contributor

@EinsteinNjoroge EinsteinNjoroge commented Sep 6, 2019

Resolves #330

Overall change: Add global styles to psammead-styles and export it without fonts

Code changes:

  • Add global styles to psammead-styles and export it without fonts
  • Add tests and snapshot

  • I have assigned myself to this PR and the corresponding issues
  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@EinsteinNjoroge
Copy link
Contributor Author

I'm not sure about this implementation, kindly confirm I'm on the right track

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.

Hey :) I see the issue says "Our font-faces should not be included initially, since they will vary depending on the service" but I would expect ${({ fonts }) => fonts && createFontStyles(fonts)} to stay in here, so we can pass the fonts in in simorgh?

They will vary on service, but we can call it like <GlobalStyles fonts={arrayOfServiceFonts} /> :)

Otherwise, this PR looks great :)

@EinsteinNjoroge EinsteinNjoroge marked this pull request as ready for review September 10, 2019 06:58
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.

Looks great, just a couple things :)

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 good so far! I've suggested some changes here.

packages/utilities/psammead-styles/CHANGELOG.md Outdated Show resolved Hide resolved
packages/utilities/psammead-styles/src/global-styles.js Outdated Show resolved Hide resolved
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.

Thanks for the changes. Suggested an update to the example.

```js
import GlobalStyles from '@bbc/psammead-styles/global-styles';

<GlobalStyles fonts={['font-family: "Sans-Serif"']}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update.

It'd be helpful to have example usage that uses our Psammead Styles fonts like this:

import GlobalStyles from '@bbc/psammead-styles/global-styles';
import {
  F_REITH_SANS_REGULAR,
  F_REITH_SERIF_MEDIUM,
} from '@bbc/psammead-styles/fonts';

<GlobalStyles fonts={[
      F_REITH_SANS_REGULAR,
      F_REITH_SERIF_MEDIUM,
    ]}
/>

Same for usage in the unit test & reference to the readme.

A note for others code reviewing this PR: A Storybook example would be difficult to isolate, so shouldn't be necessary for this work. After this package is updated we'll use it in the global Psammead Storybook setup. So it'll be in use throughout the Storybook application.

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.

@sareh
Copy link
Contributor

sareh commented Sep 12, 2019

@EinsteinCarrey After merging this in, we have two follow-up issues to use this package: #2112 bbc/simorgh#3718

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 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add global styles to psammead-styles
3 participants