Allow className to be used on the Main element

[stephencorwin]

I was using next 4.2.3. I just upgraded to 5.0.0 today and discovered that the classes that were being passed in on the <Main /> component in the _document.js were no longer coming though.

After some digging, it looks like the className pass-through was removed in 4.3.0-canary.1.

Clearly, the reason why it was removed was because <Main /> is now returning a <Fragment /> instead of a <div /> as the top-level element. <Fragment /> does not support className as a prop.
https://reactjs.org/blog/2017/11/28/react-v16.2.0-fragment-support.html

This PR aims to still allow a user to pass in classes via:

<Main className="my-class my-other-class" />

Styled Components

This is especially important in the case of supporting styled-components.

tl;dr;

styled-components requires the className pass-through for any component composed with styled().

https://www.styled-components.com/docs/basics#styling-any-components

The styled method works perfectly on all of your own or any third-party components as well, as long as they pass the className prop to their rendered sub-components, which should pass it too, and so on. Ultimately, the className must be passed down the line to an actual DOM node for the styling to take any effect.

This used to work, but no longer does:

import { Main } from 'next/document';
import styled from 'styled-components';

const StyledMain = styled(Main)`
	background: blue;
`;

Use Case

Sticky Vertical Layout

• <Header /> fixed to the top
• <Footer />sticky to the bottom (content pushed)
• <Main /> flexes

No longer works:

Before:

[timneutkens]

Lets add a test case for this. So that it won't break in the future 👍

[stephencorwin]

I'll have something in today. Thanks!

[timneutkens]

Awesome! Thanks ❤️

[stephencorwin]

Apologies. It has taken a bit longer than expected to get to this.

I opened up the test folder and was trying to decipher where everything was located. Is it just me, or does everything seem a bit "disorderly" there? At the very least, I was having some trouble getting my bearings.

Any suggestions to which file this test should reside in would be very helpful. :)

[kachkaev]

Same issue here after upgrading to 5.0.0. @stephencorwin how do you overcome the issue meanwhile?

[stephencorwin]

@kachkaev I ended up moving everything within the <Main /> element because Next.js doesn't support SSR with styled-components at the top-level.

#3796 (comment)

We definitely should still be able to use className on <Main />, but I have been very busy and haven't had time to write a test and get it through the review process (since I had to use the work around anyway). Apologies.

[timneutkens]

I still wonder why it’s needed, there is an id on the element, it’s Selectable using css 🤔

[kachkaev]

I simply solved the lack of className in Main by wrapping it into a div:
https://gitlab.com/kachkaev/website-frontend/commit/f46b14416e44080e6f0bd4c922531c33c35bedd5

Now i’m also wondering if a complication of Main in v5 is worth it given that the fix is so easy. Only those who are switching from Next v4 to v5 may get confused, and according to the number of :thumbs_up: in the PR, the number of us is not so large.

[timneutkens]

Right, so there is no real gain in adding this, since you can either:

1. wrap in a div as per @kachkaev's code
2. use #__next in your css

I'm basically trying to avoid adding something (shipping more code) for something that can be done like your suggestion 👍👌

Either way thank you very much @stephencorwin for making the PR and @kachkaev for getting involved 🙏 👍

[kachkaev]

Makes sense. If I remember correctly, the only reason why I had to have this additional div.next-main in next v4 was because I had to align the page both veritically and horizontally and just adding CSS to #__next was not enough for FF. In most cases though styling #__next will be sufficient. Having id and className in Main will not work in v5 as in v4 anyway, because both will apply to the same div, not two nested ones. In any case, cnanging CSS is necessary.

[kachkaev]

Thank you for this PR @stephencorwin – your investigation of this issue really helped me migrate to next v5!

[stephencorwin]

@timneutkens
In general, it is good practice to have HOC support the className prop. Imagine if we had an <Field type="text" /> component that didn't let you pass className in? it would not be very flexible at all in terms of styling at the call site.

For me, I find having to refer to #__next in CSS as a code smell. The CSS should not care about what JavaScript framework is the context. This causes a tight coupling. I would much rather use some selector that the CSS theme or framework determined rather than be forced to update the Theme to support using it in Next.js.

Personally, I don't usually subscribe to using Bootstrap, but ALOT of the project that I take on are already using it. If I cannot drop a class provded from Bootstrap on <Main /> without changing Bootstrap's source or imposing a redundancy of styles... that is an issue for me.

tldr – this was supported before and 4.2.3 broke this change. I am very sure that many others who have yet to upgrade to 5.x will see stylistic breakages when upgrading. Supporting this change would go a long ways towards making backwards compatibility a first-class citizen.

[stephencorwin]

In short, I'll write the test case this weekend to finish pushing it through if you will reopen the PR.

[timneutkens]

The reason the className was added was because there was no good way to select the wrapping div.

Note how here the className is set on the wrapping div which is now a Fragment, in short that means that moving the className to the __next div is as much a breaking change as removing it. And the solution is to have users wrap <Main/> themselves. Just like @kachkaev's example.

With regards to styling and eg bootstrap, the recommended way to do wrapping of all pages is https://github.com/zeit/next.js/tree/canary/examples/layout-component, since _document is also only server rendered.

[kachkaev]

The only non-breaking change would involve forcing a wrapper div inside Main, but that would be the same as not using Fragments. In most cases an extra wrapper div can bring pain because it may require styling. So in general it’s great that Fragment helps get rid of that forced extra div, while _document gives enough flexibility to add a wrapper if it’s needed.

[timneutkens]

Exactly @kachkaev 🙏

[stephencorwin]

Not totally in love with it, but I could concede. Personally, I feel as though every component that involves injecting DOM nodes, should support the className prop.

However, the example that is shown from @kachkaev + the layout component makes me think that I might be able to approach it very much like the Handlebar's {{{body}}} injection.

Thanks! 👍