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
Add typography #22
Add typography #22
Conversation
|
||
import "./index.css"; | ||
|
||
const HeadingOne = ({ children }) => <h2 className="HeadingOne">{children}</h2>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the component HeadingOne
but using a h2
is confusing to me. What was the reasoning behind this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too was confused by this (and HeadingTwo
being used on h3
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our intent is to name typographic elements agnostically to the platform. The design system itself should be applicable to other platforms e.g. a native phone app. As such, these components abstract the web platform implementation details into the component, whereas the name of the component itself is consistent with the design system.
We went with the names Title, HeadingOne, HeadingTwo etc as they common across a few different design systems we looked at (e.g. https://material.io/guidelines/style/typography.html#typography-typeface) and tools like Microsoft Word.
Also Title
has a different behavior to the headings, in that its size is bumped up (using a media query) as well as being proportionally scaled.
Finally, Title
and Lede
are likely to go together, whereas HeadingOne
, HeadingTwo
will likely go together.
We're using h2
at the moment as a default because it's the hierarchically the 2nd heading in our system. Going forward this component can be extended so that a prop can be used to specify which HTML heading element should be used. Therefore, it'll be the composing component's responsibility to specify the HTML used.
import "./index.css"; | ||
|
||
const UnorderedList = ({ children }) => ( | ||
<ul className="UnorderedList">{children}</ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain the decision behind making components like UnorderedList
,ListItem
, and Paragraph
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encapsulation of styles.
Styles are scoped to each component. It's pure at the moment, but it might not prove practical. Perhaps a TextBlock
component that styles descendant type selectors will end up being the approach taken e.g.
.TextBlock p { font: .. }
.TextBlock li { font: .. }
I'd like to see how far the pure approach can be taken, though. As it eliminates side effects in CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've a few questions (as comments). 👍
|
||
import "./index.css"; | ||
|
||
const HeadingOne = ({ children }) => <h2 className="HeadingOne">{children}</h2>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too was confused by this (and HeadingTwo
being used on h3
).
const HeadingOne = ({ children }) => <h2 className="HeadingOne">{children}</h2>; | ||
|
||
HeadingOne.propTypes = { | ||
children: PropTypes.node.isRequired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is children
the preferred name for text to go in? I see we use it in multiple components.
If it didn't break convention, I'd prefer "heading", "subHeading", "ledeCopy", or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
children
is the key provided by Gatsby React which is being destructed out from the first argument here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
children
is a special prop in react "to pass children elements directly into their output".
In each of these components, we might be passing in a word wrapped in a<b>
,<em>
or <a>
.
@@ -0,0 +1,3 @@ | |||
.HeadingOne { | |||
font: 600 1.8125rem/2rem Nova, sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a fallback between Nova
and a system picked sans-serif
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first iteration, I've kept it as simple as possible. Let's create an issue for working out what advantages a middle fallback will add.
font-family: Nova; | ||
font-style: normal; | ||
font-weight: 300; | ||
src: url(./nova-light.woff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get woff2 variants as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first iteration, I've kept it as simple as possible (i.e. one-size-fits-all). Let's create an issue for optimising the font delivery.
src/index.css
Outdated
* Foundation proportional font-sizing system | ||
*/ | ||
|
||
/* stylelint-disable selector-max-type, declaration-property-unit-whitelist */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we disabling the stylelint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to turn these rules off globally as using them in this file is an exception to the normal rules. stylelint has disable commands specifically for this reason.
For this specific block of code we're using the %
unit for font-size
and using type
selectors. Elsewhere we use rem
for fonts and use class
selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we bending the rules for this section of code though? I often find sections of code that get skipped by linters become problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add an explanation for future developers.
src/layouts/index.js
Outdated
import "./index.css"; | ||
|
||
const Layout = ({ children }) => ( | ||
<div className="Layout"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping main
inside of a wrapper div
seems wasteful, what was the reasoning for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Originally this layout file also contained Header
and Footer
components... this wrapper was left over from then. Will remove.
03c5e82
to
35a193e
Compare
<div className="Index-paragraph"> | ||
<Paragraph>{`The typographic system is:`}</Paragraph> | ||
</div> | ||
<div className="Index-unorderedList"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can remove these wrapper divs in this file? It feels like we're using way too many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach works well for apps as all components are consistently inward looking. It's the responsibility of composing component (in this case, the page) to layout its child components. At the moment the page is mainly a document and so this approach can appear verbose. I think it's worth persevering with, but we can continually assess whether it's appropriate for this site.
src/pages/index.js
Outdated
</div> | ||
<div className="Index-unorderedList"> | ||
<UnorderedList> | ||
<ListItem>{`a consistent baseline of 8sp`}</ListItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure what the sp
value means here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's swap it to px
as it turns out sp
is specific to android development.
Typography is made up of three pillars: - typographic system - suggested sizes and styles - proportional scaling Explain these three pillars in the most concise way possible with examples, whilst conforming to the GDS guidelines on content style. The suggested styles use Proxima Nova Light and Semi-Bold in conjuction with system-ui fonts to: - Adhere to the new Barnardo's digital look and feel - Optimise performance by minimising page weight Apply all these to the website itself.
35a193e
to
f152982
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
👍 It would be great to catch up about the direction of content on the site, I want to make sure it develops to explain not just what to do but also why. |
Sounds good. We're currently discussing principles which should give us an overarching why. We can then better determine how much explanation is needed at this lower depth. |
Typography is made up of three pillars:
Explain these three pillars in the most concise way possible with examples, whilst conforming to the GDS guidelines on content style.
The suggested styles use Proxima Nova Light and Semi-Bold in conjuction with system-ui fonts to:
Apply all these to the website itself.
Closes #3 #15
Ref #1