Skip to content

Conversation

@starryeyez024
Copy link
Member

@starryeyez024 starryeyez024 commented Mar 14, 2019

When trying to unravel issue #360, I noticed that in the band component, the compiled css contained this style:

::slotted(*) {
    margin-bottom: 0;
}
::slotted(*) {
  margin-bottom: var(--pfe-band--gutter--vertical);
}

which (I think?) is coming from this sass:

  @include pfe-slot($slot-name, "> *") {
    margin: 0;
  }

which lead me to wonder, is there much to gain from abstracting selectors? I was searching and searching for slotted but couldn't find it because it was @include pfe-slot() instead. So I thought maybe this is one of the levels of abstraction we can simplify? I understand the looping was to reduce replicating code, but it was also generating css selectors that don't work:
::slotted([slot="pfe-band--header"] > *:not(:last-child)) . Side note for the future, this selector (line 59) does work: [name="title"]::slotted(*:not(:last-of-type))

So, the explanation is almost longer than the code, but I wanted to explain the why for this approach, and why I removed the slot mixin. :)

@castastrophe
Copy link
Contributor

I was already planning to deprecate the slot mixin. The goal behind was not to simplify the code but rather, slots are really confusing about how to structure the selector so the goal was to abstract that so that if we find a better way to structure our slotted references, we could update that in the mixin and have it propagate everywhere. That said, I just couldn't make the mixin work well.

@castastrophe
Copy link
Contributor

So +1 slot mixin deprecation but I'd push back on removing all the loops from our sass like this. It's not that much abstraction to say, for the following regions - header, footer, body, aside - do this same thing.

@starryeyez024
Copy link
Member Author

@castastrophe when I was trying to add the fallback margins and test in IE11, I noticed two things:

  1. moving the layout styles to shadowDOM classes means IE11 cant see any of those styles (there is no shadow dom? at least the inspector doesn't think so)
  2. I can view the pfe-band demo page in IE11, in this branch, but not in master (can you reproduce this?) I wonder if the slot styles are what's breaking the band in master?

#1 has major implications in terms of how we style things and how they work (or don't in IE11) if that is true. cc @markcaron @kylebuch8

@starryeyez024
Copy link
Member Author

@castastrophe when I was trying to add the fallback margins and test in IE11, I noticed two things:

  1. moving the layout styles to shadowDOM classes means IE11 cant see any of those styles (there is no shadow dom? at least the inspector doesn't think so)
  2. I can view the pfe-band demo page in IE11, in this branch, but not in master (can you reproduce this?) I wonder if the slot styles are what's breaking the band in master?

#1 has major implications in terms of how we style things and how they work (or don't in IE11) if that is true. cc @markcaron @kylebuch8

this is what the band looks like in IE11, when i can get it to load (after i added the band, the page throws errors about a script taking too long to load)
https://www.evernote.com/l/AA62Bbjjq3JGFL5r1h2zp9DmmPqfknGC58k

…band body), update readme with explicit notes for this section, refine demo file
@starryeyez024 starryeyez024 added the ready: code review Ready for code review! label Mar 19, 2019
castastrophe and others added 2 commits March 19, 2019 16:03
…fork

Proposed optimization, returns some of the formatting and tooling
castastrophe
castastrophe previously approved these changes Mar 19, 2019
Copy link
Contributor

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of documentation notes, otherwise great +1!

@castastrophe castastrophe added needs code updates Code updates have been requested. ready to merge and removed ready: code review Ready for code review! labels Mar 19, 2019
@starryeyez024 starryeyez024 removed the needs code updates Code updates have been requested. label Mar 19, 2019
@castastrophe castastrophe added needs code updates Code updates have been requested. and removed ready to merge labels Mar 21, 2019
…patternfly-elements into issue-360/band-body-grid-gap
Copy link
Contributor

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@castastrophe castastrophe added the feature New feature or request label Mar 22, 2019
@castastrophe castastrophe removed the needs code updates Code updates have been requested. label Mar 22, 2019
@castastrophe castastrophe merged commit 6b6685f into master Mar 22, 2019
@castastrophe castastrophe deleted the issue-360/band-body-grid-gap branch March 22, 2019 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants