Showcase: Reposition the Main component (#23768)

[sixhours]

Main needs to wrap header divs or they'll persist upon exiting the section.

/cc @mendezcode @mapk

[matticbot]

Test live: https://calypso.live/?branch=fix/theme-showcase-main-markup

[sixhours]

CircleCI is failing on the Main wrapper's class name:

className should follow CSS namespace guidelines (expected themes__ prefix) wpcalypso/jsx-classname-namespace

I'm using the original class name (themes) for consistency, but maybe it should be changed to match the new standards.

[mapk]

I just tested it with Chrome inspector in responsive mode and it worked great.

As for the test failure, I'm not sure that's necessary. What you have appears to align with some of the other selectors in the markup.

Sharing

Comments

Themes

@folletto or @seear, do either of you agree with this assessment? Should we disregard the failure notification and just merge?

[sixhours]

Just noticed a bug...the header section title has disappeared when the patch is active. See the original, sans patch (under "This Site Title"):

That should probably be fixed before we merge. Looking into it now...

[folletto]

That's a part of the codebase I'm not experienced with, so I don't feel I can give you a reliable answer. Sorry!

[sixhours]

The missing section title is due to the absence of the DocumentHead component within the moved Main components in the individual files.

I haven't figured out how to bring that over, since it's dependent on a number of props and state pieces that need to be untangled from the original theme showcase file.

[seear]

Main needs to wrap header divs or they'll persist upon exiting the section.

I'm not quite certain about the premise of this PR. New content is rendered into #primary and nothing below that should persist. Is there a test case to reproduce the problem being fixed here?

[sixhours]

@seear Yes! Check out #23768 for the original report.

[seear]

Check out #23768 for the original report.

Thanks @sixhours! I'm digging in to see what has caused this...

[ryelle]

I'm not sure if we have a better way of handling this, but you can pass the className in using a variable, and that bypasses that check.

const className = 'themes';
<Main className={ className }…

the header section title has disappeared when the patch is active.

I'm actually seeing this on master, too - if you go to any section, hit the chevron to go to the menu, then click the same selection, the title disappears. I tried to fix something similar in #22501, but I thought we didn't need it anymore. I can take another look though, since it seems to be a continuing issue 🙂

[sixhours]

@ryelle Thank you for looking into this! It sounds like we're OK to continue testing this patch, with the idea we'll fix the section title bug in a separate PR?

[alisterscott]

Are we still keen to progress this PR and get it merged?

[alisterscott]

Closing due to inactivity - please reopen if necessary