WebDevStudios / wd_s

A starter theme from WebDevStudios.
https://wdunderscores.com
GNU General Public License v2.0
664 stars 138 forks source link

Audit Grid #438

Closed jomurgel closed 5 years ago

jomurgel commented 5 years ago

We've had several iterations of our grid with/without Neat. We still utilize a grid, or grid classes, for styling our theme. However, I'm not sure these are even used and/or are ignored when we get into scaffolding. I say this mostly because styles for the template-sidebar-right.php utilizes styles inside _layout.scss and not our presentational classes.

Another example: Our ACF recent posts block, which is a grid, utilizes additional styles from neat does not use our grid.

It might be good to either get on the same page with the grid and document or remove in favor of user-styling.

jomurgel commented 5 years ago

I've done some research and come to the following recommendation.

I think we should remove the grid entirely from the WD_s (Neat, custom class names and all) and replace it with a select set of helper classes or mixins.

My thought behind this is as follows:

I do think we need to handle styles for, and output of full-width and container-width elements. Out of the box, we're entirely full width, but almost always need to construct the header, content, and footer to a max-width.

I also think we could remove anything to do with the grid settings (used by Neat) since I doubt anyone even uses the Neat 1.x overlay or settings after design given the notes above.

I think we could slim down the framework and dependencies quite a bit doing this.

We might also consider creating some Mixins or presentational classes for these grids (above) and enforce the use of either grid or flexbox to keep things consistent. I think, anymore, we could and should discontinue use of floats if we don't need them for legacy support.

jomurgel commented 5 years ago

Here are my thoughts around implementing a new grid/framework for wd_s.

  1. Remove all instances of Neat and the custom grid system currently in place. They are currently on v 3.0.0 and we're using 1.9.0.
  2. Confirm the following new grid types.
    • 25% (4-up grid)
    • 33% (3-up grid)
    • 50% (2-up grid)
    • Sidebar left (33/66%?)
    • Sidebar right (66/33%?)
  3. Remove elements' need for floats.
  4. Setup globally-consistant wrapping framework for grids. Potentially test/address IE11-related flexbox issues. Needs testing.
    <div class="grid-container">
    <div class="grid-inner-container ie-fix-container">
      <!-- elements here -->
    </div>
    </div>
  5. Add mixins and/or presentational classes to render containers and widths. Explaination below.

I've given this a lot of thought. If we decide (and I think we're all on the same page with this) to use flexbox for grids we could either create mixins for containers and card types like:

.grid-container {
    @include grid-container();
}

Which might have some arguments like $equal_height = true or what have you. Perhaps the ability to add margin attributes to define the spread between elements. And then cards with mixins like:

.card {
    @include grid-card(2/3/4);
}

or

.card {
    @include grid-column(half/third/quarter);
}

Which would render a 50% width card, for example.

The other option might be to still create these mixins, but to create a series of presentational classes to render what's necessary. Where, in the example below. grid-container defines all of the flexbox container items and card defines the card attributes.

<div class="grid-container equal-height">
   <div class="card card-two"></div>
   <div class="card card-two"></div>
</div>

Obviously naming conventions are up for discussion, but as an examle.

Personally, I'm inclined to use and build mixins for use with grids rather than presentational classes since, per previous discussions, can and will muddy up the markup.

I think a simple set of classnames should be setup (that could be reused) in wd_s out of the box, but would only by a wrapper class and card class perhaps.

I would also like to get #382 merged into master before we do any work here to take advange of these updates without potential merge conflict issues.

mharis commented 5 years ago
  1. I haven't seen Neat being used in the default wd_s anywhere along with the custom grid. From the custom grid, I have only used max-width variable once in the recent project and never used Neat in any of the projects I've done so far.

  2. Agreed with the grid specifications. I haven't implemented 1/3 + 2/3 columns in my projects yet but if that is a usual case at wd_s, we might want to think about implementing some of the common use cases which would help save time during the scaffolding process.

  3. Agreed, we should remove floats and use flexbox. IE11 gracefully degrades with flexbox.

  4. I used CSS grid for Maintainn but we needed IE11 support so I decided to use Flexbox. Considering that our columns system is super simple, we will not have too much trouble with using flexbox and also supporting IE11. Whoever implements it should consider reading "Known Issues" section at https://caniuse.com/#feat=flexbox

  5. I thought about mixins vs presentational classes and I cannot come up with a situation where we cannot use a mixin and need a presentational class. My vote is for mixins as it will keep us from polluting the markup.

coreymcollins commented 5 years ago

I really love everything I'm seeing here! I do want to take some time to think through some of our options, but out of the box I lean toward mixins over presentational classes as well. I don't tend to have a problem with them in general, and I'm sure we'll use them here and there on non-grid things from time to time, but if we can keep our markup free of needless classes then I think that's the way to go.

We also need to be sure that we're creating mixins that are powerful, yet not confusing. We don't necessarily need to cram everything into one mixin either. If one mixin starts to grow out of control, then perhaps we have one for setting up the grid, one for setting margins/padding, etc. with some nice defaults set at a global level.

Not using presentational classes will save us time in the long run, I think. If we stick with simple class names like grid-container and card or even cell or something, we only need to remember two classes. We don't need to try and think of which class names make this a 2x grid, or a 4x grid, or a content-sidebar layout – we'll be able to provide the base markup and alter as needed. I think this could help cut down on time spent making and styling grids and cards the more we develop and use it.

Like I said, I want to give this some more thought before weighing in fully and we can all chat about it on scrum next week to gameplan some more.

Great stuff!

jomurgel commented 5 years ago

Adds branch feature/#438-update-grid

jomurgel commented 5 years ago

Doing an audit of the current layout and grid system in place in the PHP also.

Right now, on a page with all elements our layout would look like this:

<body>
    <div id="page" class="site">
        <header class="site-header">
            <nav></nav>
        </header>
        <div id="content" class="site-content">
            <div class="primary content-area">
                <main id="main" class="site-main">
                    <article>
                        <!-- Standard Post Elements -->
                    </article>
                </main>
            </div>
            <aside class="secondary widget-area col-l-4" role="complementary">
                <!-- Standard Widget Elements -->
            </aside>
        </div>
        <footer class="site-footer"></footer>
    </div>
</body>

I question the need for the parent wrapper and content wrappers. Something like this seems fine with what we're working with typically.

<body class="site">
    <header class="site-header"></header>
    <main id="content" class="site-content">
        <section>
            <article><!-- Post Content --></article>
        </section>
    </main>
    <aside class="secondary widget-area col-l-4" role="complementary">
        <!-- Standard Widget Elements -->
    </aside>
    <footer class="site-footer"></footer>
</body>

That said, in researching "best practices" in 2018+ it seems like we might want to look more deeply into a few items in use in WD_s as well specifically the use of <article>, <section>, and <div> in building out a site.

Since <section> is a semantic tag it should only be used to split up related, but not necessarily self-contained content and shouldn't be used in replace of a <div> tag. This then calls into question our default ACF blocks which all, last I checked, utilize the section tag, but all potentially make up the main article content for a page. In this case they should be divs inside an article, and/or somewhat flexible.

I've added a new issue #445 to potentially address these issues.

jomurgel commented 5 years ago

Another found point of reference that says similar things:

jomurgel commented 5 years ago

EOD Update.

To Do

I'd like to also do an audit of mixins and such which we can probably do as part of #443, but I'm coming across a bunch that are either not in use or outdated or broken (full-width mixin for example — which will be removed in favor of full-width class structure I've added).

jomurgel commented 5 years ago

Making several updates here with a bunch of opinions, but the broad strokes are.

Still, a bunch to do:

jomurgel commented 5 years ago

Issued a PR #447

Additional doc: https://docs.google.com/document/d/1o1zfqeT5bcdu4VsCHCOKCbO72YMKjb7509I83srkzKU/edit#