pookmish commented 1 year ago

READY FOR REVIEW

Summary

Change the way the .centered-container works. The current solution only works at the outer most container element, often children of body or such. Otherwise you get gutters inside gutters inside gutters. This change allows the user to nest the classes and properly set the element to have appropriate gutters at all breakpoints.

ex:

<div class="cc">
   <div class="cc">
    <div class="cc">
       this element would be 450px inset from the window on each side while on 2xl screens, and 75px on each side on mobile. The proposed change would make sure this text would only have 25px on mobile and 150px on 2xl.
    </div>
  </div>
</div>

sherakama commented 1 year ago

I like this change in behavior as it matches my expectation of how it should behave and how other CSS libs behave. A couple of questions in here first.

pookmish commented 1 year ago

I did this implementation and it's so much easier to just set the class on the drupal paragraph and I never have to worry about the container creating padding or what it's size or left/right side contains.

yvonnetangsu commented 1 year ago

@pookmish @sherakama Was in meeting this morning so I need to catch up with the full thread of conversations, but, we did have a reason that we used the padding method for the existing su-cc class - it's a long story due to having it exactly match the Decanter v6 centered-container mixin and it's because of UComm's request for that approach.

Basically, using existing su-cc, it would allow one to do this

<div class="su-cc su-bg-digital-red">
Some text
</div>

It will create the max width container around the text, but still have the background color shown as edge to edge. UComm wanted to use as few nested container as possible so that's why we did it that way (instead of using outer container with bg color, and inner container with su-cc).

So, because of that history, directly modify the existing su-cc class would break backward compatibility for our sites 😬 . Are you open to adding your approach as su-cc-alt perhaps @pookmish?

yvonnetangsu commented 1 year ago

@pookmish Personally I'd use your approach too if it's not because of backward compatibility and the UComm request back then. cc @sherakama

pookmish commented 1 year ago

So, because of that history, directly modify the existing su-cc class would break backward compatibility for our sites grimacing . Are you open to adding your approach as su-cc-alt perhaps @pookmish?

Yet again, poor implementation for an obscure use with a single site due to legacy issues. Feel free to do what you wish to my PR. I'll just keep overridding more and more or using less of Decanter in my projects.

yvonnetangsu commented 1 year ago

So, because of that history, directly modify the existing su-cc class would break backward compatibility for our sites grimacing . Are you open to adding your approach as su-cc-alt perhaps @pookmish?

Yet again, poor implementation for an obscure use with a single site due to legacy issues. Feel free to do what you wish to my PR. I'll just keep overridding more and more or using less of Decanter in my projects.

It's not a single site. We have half a dozen live sites using decanter v7 and making this change would require us to test all those sites and make changes if needed.

sherakama commented 1 year ago

Yet again, poor implementation for an obscure use with a single site due to legacy issues. Feel free to do what you wish to my PR. I'll just keep overridding more and more or using less of Decanter in my projects.

@pookmish here's your tiny violin.

@yvonnetangsu, This sounds like we have two different use cases here and we have a need for a centered banner and a centered chromeless. And to add both, we would have a naming issue. the centered container should be the chromeless option IMHO but we should support the banner too.

Can we do both and come up with class names that make sense? I'm ok for a find-all-and-replace type change.

pookmish commented 1 year ago

Ok, I won't be making contributions any longer. There's too much difficulty to make improvements.

sherakama commented 1 year ago

@pookmish

Thank you for meeting with me and talking this through. I apologize again for the inappropriate giphy but I am hoping you can write out a few answers to some of the questions we talked about and a couple of new ones to document your challenges.

What would you have liked the outcome of this PR to be if straight acceptance was not possible or if a disagreement on the legitimacy of existing use cases?
How does the suggested compromise in this thread add technical debt? How does it negatively affect your work?
How does "overridding more and more or using less of Decanter" negatively affect your work?
What are your tradeoffs to abandoning Decanter? ie: what do you stand to gain/lose?

You also mentioned that overriding individual class names isn't possible and that it creates a merge of styles. I've flagged that as something to look into.

thanks,

jbickar commented 1 year ago

@pookmish

Thank you for meeting with me and talking this through. I apologize again for the inappropriate giphy but I am hoping you can write out a few answers to some of the questions we talked about and a couple of new ones to document your challenges.
* What would you have liked the outcome of this PR to be if straight acceptance was not possible or if a disagreement on the legitimacy of existing use cases?

* How does the suggested compromise in this thread add technical debt? How does it negatively affect your work?

* How does "overridding more and more or using less of Decanter" negatively affect your work?

* What are your tradeoffs to abandoning Decanter? ie: what do you stand to gain/lose?
You also mentioned that overriding individual class names isn't possible and that it creates a merge of styles. I've flagged that as something to look into.

thanks,

@pookmish, please respond in this thread.

sherakama commented 1 year ago

Hey @pookmish,

We'd like to wrap this up with you and need your response. Please provide your response to the final questions and we can close this out and move along.

sherakama commented 1 year ago

@pookmish It's been more than two weeks and I haven't heard back from you. Please wrap this up by answering the remaining questions. Thank you.

sherakama commented 1 year ago

@pookmish this is a reminder about the thread you agreed to answer when we met in our 1:1 on Tuesday. Please answer the questions in https://github.com/SU-SWS/decanter/pull/911#issuecomment-1561620893

SU-SWS / decanter

Change centered-container to use max width and allow nesting of classes #911

READY FOR REVIEW

Summary