Skip to content
This repository has been archived by the owner on Dec 6, 2022. It is now read-only.

RFC: Grids and page layout #488

Closed
SiAdcock opened this issue Aug 10, 2020 · 6 comments
Closed

RFC: Grids and page layout #488

SiAdcock opened this issue Aug 10, 2020 · 6 comments
Labels
Grid Changes to the grid component Request for comments 📣 A proposal or proof of concept. Comments from a wide audience would be appreciated!

Comments

@SiAdcock
Copy link
Contributor

SiAdcock commented Aug 10, 2020

The most-requested feature for Source at the moment is support for Guardian's grid layout system. This includes abstractions around:

  • common media queries
  • the number of columns available at each breakpoint
  • the width of columns at each breakpoint
  • the width of gutters between columns
  • whether a border appears between columns
  • horizontal padding around the page container

The abstraction must ensure the layout doesn't look broken in IE11.

Note: this request is not simply about providing a convenient wrapper around CSS Grid. It must consider the Guardian's grids, with the purpose of unifying layout across products.

Approaches

Component-based

We are currently developing a component-based approach with an API that looks like:

import { GridRow, GridItem } from "@guardian/src-grid"

const Article = () => (
    <GridRow breakpoints={["mobile", "tablet", "desktop", "leftCol", "wide"]}>
        <GridItem span={[0, 3, 3, 4, 4]} borderRight={true}>
            <Sidebar />
        </GridItem>
        <GridItem span={[4, 9, 9, 10, 12]}>
            <Main />
        </GridItem>
    </GridRow>
)

See the Grid component API docs for more details.

Pros

  • Development is easier as the JSX reflects the layout. This might be useful for maintainers reading existing through code.

Cons

  • Adds extra HTML elements to the generated markup which has performance implications
  • Makes JSX more verbose when reading, and distracts from the actual content of a component
  • See RFC: <Text> component #423 (comment) for very related thoughts

CSS maker functions

An alternative approach uses functions to generate CSS that are included directly in the css tag:

const gridContainer = css`
  ${grid({
    grids: [
      'wide',
      'leftCol',
      'desktop',
      'tablet',
      'mobile'
    ],
  })};
`

const leftColumn = css`
  ${gridItem({
    columns: [
      3, // wide
      2, // leftCol
      3, // desktop
      0, // hide on tablet
      0, // hide on mobile
    ],
    offset: [
      2, // offset 2 columns to the right at wide
      0, // leftCol, don't offset
      0, // desktop, don't offset
      0, // tablet, don't offset
      0, // mobile, don't offset
    ]
  })};
`

Manage Frontend provides a similar approach but with less granular control over breakpoints. DCR uses a a CSS maker approach but using grid-template-columns and grid-template-areas.

Pros

  • No extra markup generated
  • Portable to non-JSX projects. Feasible to translate into SCSS

Cons

  • Perhaps less readable?

Discussion

I would like to open for comments which of these approaches is the most likely to provide the most coherent, maintainable and extensible developer experience without detrimentally affecting the client side performance.

We are not discussing implementation or the exact API at the moment. We can discuss whether either approach unlocks the possibility for a particularly useful API.

I'd love to hear your thoughts 🎉

Also if you have tried out the Grid components provided by Source, please let us know your experience.

@SiAdcock SiAdcock added Grid Changes to the grid component Request for comments 📣 A proposal or proof of concept. Comments from a wide audience would be appreciated! labels Aug 10, 2020
@adamnfish
Copy link

adamnfish commented Aug 10, 2020

I think this comes down to a question of what your team's goals are and how different you feel the usability of these two approaches are today.

If the usability of each is similar then there's no benefit to tying yourself down*. You're probably better off sticking with the more flexible option. There's nothing to stop you from adding tooling or helpers on top to make life easier for React users. However, it sounds from your description like your team feels the component API is better. It's great for us to standardise on an approach and then create a tailored set of tools that make like as easy as possible for that use-case so maybe it's worth using the more bespoke API. In this case we need to consider your goals to help make the decision.

Do you want Source's Grid support (or perhaps we can talk about the Source library more generally) to be the canonical system that everyone uses, or do you want it to be an easy option for teams to use today? What about tomorrow - and how much work do you think the department should set aside to keep up with tomorrow's goals? I don't mean for these to be leading questions, these are important topics and I am interested to hear everyone's views here!

There seems to be great consensus on React today, but these things tend not to stick around forever. If the grid component's API is a React one, then we have to add a new API in the future when a new technology appears in the department. If the grid component's API is one that is not tied to a framework then its API stays the same as technologies come and go here.

These are (mostly) hypothetical concerns today though. Maybe we aren't worried at all because we can add another approach as it is needed while keeping things as simple as possible today.

Interested to see the discussion on this, I think there's lots for us all to learn from what gets decided here. Following the Engineering All Hands yesterday, it sounds like we are a department facing renewed investment in our technologies. Should we be focusing on making life simple today, or encouraging innovation outside of our existing technology stacks? There's no right or wrong answer!

* this is only true because Source is a library designed to be used widely by other teams that are free to take different approaches in their own applications. There would be huge benefit in tying ourselves down if we were building a concrete app here!

@nicl
Copy link
Contributor

nicl commented Aug 11, 2020

Thanks @SiAdcock for starting the conversation!

I'm keen on the Component-based approach. Here's why:

Most of the time with our React/Emotion/Source/Typescript ('REST') stack, we want components that are isolated. This would appear to favour the CSS maker functions approach. However, layout works against this; components push each other around, and the key CSS ways to do layout typically require attributes on multiple components, for example in flexbox you might need a display:flex on the wrapper, and align-item on the items, etc. As a result, you want some way to easily 'see' the whole picture, and rearrange things. The Component approach achieves this, albeit at the expense of extra markup, whereas the CSS maker functions approach doesn't do so well here.

@tompretty
Copy link

Yeah I'm also in favour of the Component-based approach. I would argue (perhaps incorrectly) that a component like sidebar shouldn't know how many columns it spans. That should be the responsibility of its parent. I think that helps to maximize reusability (e.g. having different layouts which show/hide the sidebar at different breakpoints). In which case, with the css approach you'd still have a bit of extra markup e.g.

<div className={leftColumn} >
  <Sidebar />
</div>

At which point, I think I'd rather that just be more explicit like in the component approach.

I think also as we're expanding on our layout components, if we had something like <Column />, it might be a little odd to then have the grid being purely css.

@i-hardy
Copy link
Contributor

i-hardy commented Aug 12, 2020

While I think it would be nice to have the CSS functions as an option, I strongly favour the component approach as the primary API. I agree with @tompretty that it's often desirable to decouple a component from its position in a layout so we wouldn't necessarily be saving on markup in any case, and the grid components make the layout intent very clear and declarative for the reader without having to pour over CSS strings.

Also as I mentioned in the meeting this morning, it's worth keeping in mind that a JSX implementation doesn't necessarily tie us to React- JSX is already supported by Vue, and can be utilised by pretty much any function with the same signature as React.createElement.

@gtrufitt
Copy link

+1 on the component approach. I think that it feels far more meaningful and the css-maker approach in DCR is less understandable than if we were to refactor to use the component approach.

The component approach is easier to reason with and is more aligned to keeping naturally-coupled concepts together in the code, making it easier to visualise the layout by reading the code.

@SiAdcock
Copy link
Contributor Author

SiAdcock commented Sep 9, 2020

@adamfish

There are some really useful questions here, thanks so much for engaging. I think it’s reasonable to build layout components using the CSS maker approach, and then providing a React wrapper that teams could use if they would find it useful. I’m keen to take advantage of JSX, the ergonomics of which is the main reason a lot of our projects are now built on React. If a better framework or templating engine came along in the future, it’s likely we’ll have to re-build these components. However, we could reuse the same underlying CSS functions and expose a similar API.

Some choice quotes here:

@nicl

However, layout works against this; components push each other around, and the key CSS ways to do layout typically require attributes on multiple components

@tompretty

I would argue […] that a component like sidebar shouldn't know how many columns it spans. That should be the responsibility of its parent.

@i-hardy

JSX implementation doesn't necessarily tie us to React

@gtrufitt

[The component approach makes it] easier to visualise the layout by reading the code

After spiking layouts for a couple of days I’m closing this RFC in favour of a slightly improved approach based on smaller, more composable components: #526.

Thanks so much for all your feedback. Hope to see you on the next thread 😄

@SiAdcock SiAdcock closed this as completed Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Grid Changes to the grid component Request for comments 📣 A proposal or proof of concept. Comments from a wide audience would be appreciated!
Projects
None yet
Development

No branches or pull requests

6 participants