Skip to content
This repository has been archived by the owner on Sep 16, 2019. It is now read-only.

xy-grid, section and footer widgets #1180

Closed
jtmcgraw opened this issue Dec 8, 2017 · 9 comments
Closed

xy-grid, section and footer widgets #1180

jtmcgraw opened this issue Dec 8, 2017 · 9 comments
Labels

Comments

@jtmcgraw
Copy link

jtmcgraw commented Dec 8, 2017

Footer widgets weren't expanding to full width at medium and small breakpoints.

I added grid-x to <footer class="footer"> in footer.php --- didn't work
I replaced large-4 column with large-4 cell in <section class=""> in /library/widget-areas.php --- still didn't work
Wrapped <section> in <div class="large-4 cell"> in /library/widgert-areas.php --- working for me, but maybe not ideal...

@colin-marshall
Copy link
Collaborator

colin-marshall commented Dec 8, 2017

@jtmcgraw thanks for the report. This is related to #1151 and #1159.

Currently the footer is like this:

<div class="footer-container">
    <footer class="footer">
        <?php dynamic_sidebar( 'footer-widgets' ); ?>
    </footer>
</div>
.footer-container {
  margin: 0 auto;
  border-top: 1px solid $medium-gray;
  margin-top: rem-calc(60);
}

.footer {
  @include xy-grid-container;
  @include xy-grid;
  padding: rem-calc(30) 0;

  section {
    @include xy-cell(auto);
  }

  ul {
    list-style-type: none;
    margin: 0;
    padding: 0;
  }
}

I propose we change it to this:

Footer Option 1

<footer class="footer-container">
    <div class="footer-grid">
        <?php dynamic_sidebar( 'footer-widgets' ); ?>
    </div>
</footer>
.footer-container {
  @include xy-grid-container;
  border-top: 1px solid $medium-gray;
  margin-top: rem-calc(60);
}

.footer-grid {
  @include xy-grid;
  padding: rem-calc(30) 0;

  section {
    @include xy-cell(auto);
  }

  ul {
    list-style-type: none;
    margin: 0;
    padding: 0;
  }
}

One thing I'm not sure about is whether the footer element should be on the outer element with the container class or on the inner element with the grid class. I personally prefer having the header and footer elements as direct children of the body.

@olefredrik @JPOak @Aetles and others, what do you guys think?

@colin-marshall
Copy link
Collaborator

colin-marshall commented Dec 8, 2017

@jtmcgraw if you then want to have 3 footer columns on large that are 1 column on small and medium then you would take what I have above and change section in the scss to this:

section {
  @include xy-cell();

  @include breakpoint(large) {
    @include xy-cell(4);
  }
}

@JPOak
Copy link
Contributor

JPOak commented Dec 8, 2017

@colin-marshall Your way is fine and a good change. However, from just eyeballing your structure, more often than not I will personally move the container inside, because I will want a fluid/full-width background color on the footer with the content centered in a container. However, that is just me and I can do that myself.

@colin-marshall
Copy link
Collaborator

colin-marshall commented Dec 8, 2017

@JPOak that's a good point and probably the most common use case. Maybe we should do this then?

Footer Option 2

<footer class="footer">
    <div class="footer-container">
        <div class="footer-grid">
            <?php dynamic_sidebar( 'footer-widgets' ); ?>
        </div>
    </div>
</footer>
.footer {
  background-color: #FFF; // or whatever
}

@JPOak
Copy link
Contributor

JPOak commented Dec 8, 2017

@colin-marshall That would be better for me!

However, your first option better aligns with how you are implementing the container class across the entire theme and templates and would be more consistent. Remember this #1146 question I had? I was making an ACF flexible content field that allowed for full width background sections. I probably would of preferred to put it in the main-wrap but decided to just add it after. These days you have lots of design that have alternating fullwidth background sections.

@JPOak
Copy link
Contributor

JPOak commented Dec 15, 2017

@colin-marshall Hey, are you waiting for more comments on this or would you like me to do a PR for this? Thinking as it is a bug maybe is should be squashed.

@colin-marshall
Copy link
Collaborator

@JPOak I was hoping for more discussion but I think option 2 can be implemented now. It can always be changed later.

Your second to last post had me thinking that it might be good to add in some module classes for full-width background sections with the container and grid as child elements. I can't think of a site I have done recently that doesn't use these.

@JPOak
Copy link
Contributor

JPOak commented Dec 15, 2017

@colin-marshall Yeah. Maybe interesting. I did a test run on a small budget project and Foundation Press out of curiosity. I was gauging how fast I could bang this out to make it worth my while at this budget range. It went alright, although I think there was some margin peculiarities that were fixed with the separation of the container classes. I guess it is always a balance between "bells and whistles" and keeping it a super clean starter theme.

@colin-marshall
Copy link
Collaborator

What we could do is make it a module, and then disable it by default by commenting it out in app.scss. That way, for people who would never use it, it's just a file and it doesn't add any bloat to their CSS. A happy medium between simple, clean code and nice-to-have features.

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

No branches or pull requests

3 participants