Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(logo-grid): support customizing the grid through variables #30

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

oxyc
Copy link
Member

@oxyc oxyc commented Aug 25, 2020

I hate this grid! :D Insanely complicated to create an internal border!

  1. What was used before is currently a bit broken and you can see it in the current storybook.

Screen Shot 2020-08-25 at 14 21 09

I was planning to fix it by using nth-last-child selector together with ~ selector but apparently you cannot use ~ with ::slotted. Only compound selectors and no combinators are allowed.

Before I reached that conclusion I also learned that sass doesnt support & selector within ::slotted() (makes sense). There's a proposal being worked on for it and meanwhile material design does stuff like this.

  1. Next consideration was grid-gap: 1px, where we could set a background color on the grid, and then override it on the cells. Doesn't work since empty cells will have the background color of the grid. We could fix it in the Stencil component but not in WP.

  2. Next up, I thought about adding an absolute positioned ::after element over the entire grid with touch-events: none that would have a border that overlaps the external border of the items. But there's no way of automatically setting the correct border-color of this pseudo element to match the background color. This could still be a valid solution if we cant hide the overflow. In WP it would work since we set --block-background when a block changes the background color, but it's sort of nasty.

  3. Finally I settled with negative margins and overflow: hidden. Downside is that future variations eg hover styles cant overflow. Also due to using this approach we cannot use auto-fill columns since the width of the grid container wouldnt be known, and easily become larger than the grid, thus showing the border. This is fine I think and also applies to the previous solution.

Oh and I used minmax for the grid column sizes so that if it does need to scale down, it will. Previously it was overflowing the viewport on small screens.

WP implementation using Gallery block is:

@use '~genero-design-system/src/components/gds-logo-grid' as grid;
@use '~genero-design-system/src/components/gds-logo-grid-item' as item;

.wp-block-gallery.is-style-logo-grid {
  .blocks-gallery-grid {
    @include grid.base;

    margin-left: auto;
    margin-right: auto;
  }

  @include mq($from: medium) {
    @for $i from 1 through 8 {
      &.columns-#{$i} .blocks-gallery-grid {
        --logo-grid-item-count: #{$i};
      }
    }
  }

  .blocks-gallery-item {
    @include item.base;

    // Increase specificity to override Gutenberg selector
    // Sass needs `&&&` like styled components have.
    &.blocks-gallery-item.blocks-gallery-item {
      @include grid.item;
    }

    width: auto; // override Gutenberg

    figure {
      @include item.container;
    }

    img {
      @include item.image;
    }
  }
}

Let's hope we eventually get w3c/csswg-drafts#2748

@oxyc oxyc requested a review from taromorimoto August 25, 2020 17:35
@taromorimoto
Copy link
Contributor

Yeah, it feels like all those host and slot styling issues are not very polished in the web component implementations. You can see this in developer tools when inspecting these web components.

Btw, @oxyc what's the reason you seem to want to use the gds-logo-grid with just the styles in WP? Could it have just worked by just using gds-logo-grid web component directly in WP? Or did I understand correctly.

@oxyc
Copy link
Member Author

oxyc commented Aug 26, 2020

Could it have just worked by just using gds-logo-grid web component directly in WP? Or did I understand correctly.

It could have, but now we can reuse the Gallery block instead of having to create our own. At some point maybe we can have our own block but I think it's good to have all our mixins flexible enough to be able to apply in other places.

@oxyc oxyc merged commit d95a09b into master Aug 26, 2020
@oxyc
Copy link
Member Author

oxyc commented Aug 26, 2020

Mostly I don't want to deal with arrays of objects with attributes in a custom maintained block :D it gets a bit messy. We can build it later

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

Successfully merging this pull request may close these issues.

2 participants