Skip to content
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

Typography [fixes #763] #801

Merged
merged 20 commits into from
Mar 14, 2020
Merged

Typography [fixes #763] #801

merged 20 commits into from
Mar 14, 2020

Conversation

carlfairclough
Copy link
Contributor

Updated all typography

Description

Moved almost all typography rules to live in T.vue which can either be called with <T props /> or with classes (unscoped). It follows tailwind/tachyons/atomic/etc style css rules with low levels of specificity. This is a highly rigid approach but enables us to ensure that type goes through a single funnel, and minimizes custom styles.

  • Default type color classes have been defined which will switch on darkmode without having to redefine them.

  • Markdown content is styled with a chain of :not()s which prevent the styles from overriding any explicitly set. For example, <div class="featured" /> has been replaced in markdown with the markdown-it-attrs package, allowing you to pass something such as
    # Lorem ipsum si dolor et amet {.l3}, which will create a h1 element with level 3 styles applied. By default, h1 uses level 1``.l1

  • Some pages with repeated content now use v-for and and object array in data: to allow styles for the content blocks to be defined once. /build/ is the most significant example of this.

Related Issue

#763

Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Hmmm this looks super neat! I do wish we had chatted through this approach before going this route though. My gut reaction is that I don't love this approach but I'm open to hearing out why you decided to go this direction. What are the advantages you see in this T.vue component + tailwinds style vs. just editing the existing global styles in theme.styl?

My primary concern is that it may create an intimidating codebase that will require a learning curve for most developers looking to contribute. E.g. I suspect a component like <T p l4 cText s300 class="subtitle mw55ch"> will be difficult to understand at first glance for most developers who are not already familiar with the codebase.

When considering tradeoffs, I'd like us to optimize our codebase for ease of contributions. I want to ensure we lower the friction as much as possible for anyone looking to hop in & e.g. complete a Gitcoin bounty.

docs/dapps/index.md Show resolved Hide resolved
@carlfairclough
Copy link
Contributor Author

@samajammin I totally understand where you're coming from, it is a lot to get to grips with initially. But imo this has a number of benefits, mainly:

No inheritance
No type element should have text size set on the parent (or be overriding a text size. It means that we can see in the codebase, in the individual page template, the rules being applied. When somebody is just using markdown, the styles map like so:

  • h1: l1
  • h2: l3
  • ...
  • h6: l6
  • p: l7
  • ul li: l7

I think a reason it looks intimidating is the chain of :not()s, which was the fastest way to ensure that any levels set take precedence over any html tag (# title text is l1, however `# title text {.l3} would be level 3)

High visibility & works according constraints
Style rules don't need to be abstracted away to a style tag when using this, and especially not away to a collection of styles (e.g. theme.styl, dark.styl, etc.). It means pages/components only need to be edited in one place. The props & classes also act as guard rails to help people leverage and work with the system.

Now there is definitely a downside with atomic classes:

  • They're almost a return to inline styles, especially when very clear class names are used (almost 1:1 with the styles)
  • There is a learning curve when shortened class names are used (like the ones here)
  • it can get messy when there are long chains of classes

The naming could be much clearer, but it is a 1:1 representation of the Figma file, allowing people to leverage the exact colours and type sizes (and max-widths) without having to apply create any extra classes. In most pages, like Build, the component + props are used. However, in Markdown, I've used {.classnames} which achieve the same thing. The component can be used there too though!

It's a little extra work, but I think all of this could be documented very easily — I could fork the repo to have a single page which shows <T (or a progression of that naming), alongside every l1 l2 l3 l4 l5 l6 l7 l8 prop/class (level1 instead?) and do the same for text colors, background colors, margins, and so on.

Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Solid progress here. Discussed this offline with @carlfairclough but circling back here - let's keep the utility classes but remove the T.vue component in favor of just rendering elements directly & using classnames instead of passing props. So instead of:
<T p l4 cText s300 class="subtitle mw55ch">
We'll do:
<p class="subtitle mw55ch l4 c-text s300">

This will make code clearer for contributors & remove the risk of developer error e.g. by passing in an invalid element to the T.vue component.

The styles in T.vue we can just move over to theme.styl or utils.styl or whever you think makes the most sense.

Thanks!

docs/.vuepress/components/T.vue Outdated Show resolved Hide resolved
docs/.vuepress/components/T.vue Outdated Show resolved Hide resolved
@samajammin
Copy link
Member

One change I noticed to be aware we account for:
Image 2020-03-09 at 2 53 36 PM
Looks like H1s now render an anchor link, which we want to avoid (i.e. keep the current format)

@samajammin
Copy link
Member

More changes to be aware of, looking at site preview:
Image 2020-03-09 at 3 05 58 PM
Spacing of the H2s looks tight.

@samajammin
Copy link
Member

samajammin commented Mar 9, 2020

More changes to be aware of, looking at site preview:
Image 2020-03-09 at 3 08 04 PM
Headers are now covered by nav when scrolling to anchor text.

Also an issue with /languages page:
Image 2020-03-09 at 3 09 28 PM

@samajammin
Copy link
Member

Another change on /languages page, card rows contain 2 vs 3 items:
Image 2020-03-09 at 3 11 26 PM

@carlfairclough
Copy link
Contributor Author

carlfairclough commented Mar 11, 2020

All issues have been addressed, however some fairly significant changes have been introduced, the most major structurally, HTML/CSS wise, around handling content blocks within markdown.

Anchors & headers

h1s now render an anchor link

These have been removed — I previously thought they made sense given they are accessible via the sidebar.

Headers are now covered by nav when scrolling to anchor text.

The targets now have pseudo-elements which prevent them from being hidden behind the nav when automatically scrolled to:

  &:target:before {
    content: "";
    display: block;
    height: 60px; /* fixed header height*/
    margin: -60px 0 0; /* negative fixed header height */
  }

This brought in a secondary issue, where the sidebar links would ignore the new pseudo-target. I have addressed the issue, however it involved an update to how the sidebar links work — instead of using router-link there is now an a instead, and I cut all unnecessary styles in the process.
I've applied the new text styles and stripped all custom css.

Spacing of the H2s looks tight

Updated this so it can breathe. We should also update figma to reflect the change

.md content column

Another change on /languages page, card rows contain 2 vs 3 items

Currently, a max-width/width is defined on the content column, with components being called inside that (which is the case inside the languages page). To avoid md components from being constrained this way, a max-width has been applied as a base style to all text elements. I imagine there may be niggles in the future (e.g. using non-text elements in .md), however right now everything appears good and working, and it plays nicely with .has-sidebar pages.

T.vue

Gone! But the utility classes are now heavily used

Utility class generation

There is a helper function to avoid repetitive class creation, it can be used in either of the following ways:

styles = {
   '100': 25px,
   '200': 50px,
   '300': 50vw,
}

generateClasses('classname-', '-custom, 'height', styles)
/// OUTPUT
.classname-100-custom { height: 25px }
.classname-200-custom { height: 50px }
.classname-300-custom { height: 50vw }

stylesAlt = {
   unit: 'em'
   values: {
     '100': 25,
     '200': 50,
     '300': 75,
   }
}

generateClasses('classname-', null, 'max-width', stylesAlt)
/// OUTPUT
.classname-100 { max-width: 25em }
.classname-200 { max-width: 50em }
.classname-300 { max-width: 50em }

Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Style changes look awesome for the most part but the component refactors to use data & template loops introduced some bugs. It will be fastest to hop on a call & talk through this.

I think the takeaway here is to move towards smaller, more iterative PRs, e.g. creating a standalone PR that refactors the components to use data & loops (& test that works across languages) vs. bundling those into these style changes.

@@ -277,5 +145,78 @@ table
border 1px dotted $colorPrimary
box-shadow 0 4px 8px $boxShadowHoverColor

// BASE TEXT STYLES
// These target mainly our markdown content, and whenever a .l# class is not used
// They should eventually be moved out of utils.styl
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean moved into utils.styl?

Copy link
Member

Choose a reason for hiding this comment

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

// They should eventually be moved out of utils.styl

h1, h2, h3, h4, h5, h6, p, span, li {
max-width: $contentWidth;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, should this always be the case?

E.g. noticing the H1 on the homepage is now 2 lines instead of 1.

Now:
Image 2020-03-11 at 2 43 47 PM

Previously:
Image 2020-03-11 at 2 43 56 PM

Perhaps we over-ride for those exception cases?

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts @carlfairclough?


// Set text colors
// these should be revisited
// alongside colors in config.vue
Copy link
Member

Choose a reason for hiding this comment

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

Ya shouldn't we use the variables from config.styl here? e.g. $colorPrimaryDark800 instead of mix($colorBlack, $colorPrimaryDark, 60%) again?

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts @carlfairclough ?

docs/.vuepress/theme/styles/theme.styl Show resolved Hide resolved
docs/.vuepress/theme/styles/theme.styl Show resolved Hide resolved
docs/.vuepress/theme/styles/utils.styl Show resolved Hide resolved
docs/.vuepress/components/HomePage.vue Outdated Show resolved Hide resolved
docs/.vuepress/components/HomePage.vue Show resolved Hide resolved
docs/.vuepress/components/HomePage.vue Show resolved Hide resolved
docs/.vuepress/components/BuildPage.vue Show resolved Hide resolved
@samajammin
Copy link
Member

Homepage is still broken for some translations, e.g. https://deploy-preview-801--ethereumorg.netlify.com/se/

@samajammin
Copy link
Member

Homepage highlight is broken for right to left languages, e.g. https://deploy-preview-801--ethereumorg.netlify.com/ar/

Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Nice work! I'm going to merge this to keep us moving forward. I will make some fixes in a pending PR. I also made a couple requests for updates / comments - please incorporate those into future PRs.

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

Successfully merging this pull request may close these issues.

2 participants