UTCWeb / particle

A starter kit for using the prototyping tool, Pattern Lab, in tandem with a Drupal theme. Utilizes Webpack for all asset management.
https://phase2.gitbook.io/frontend/
GNU General Public License v2.0
0 stars 10 forks source link

Particle configuration update for taillwind to addd !important to its classes. #171

Closed bmartinez287 closed 2 years ago

brittrich commented 3 years ago

@bmartinez287 I've been doing some thinking/researching around this and the idea of upgrading to Particle 2.0 and have a few thoughts (do remember, I've been out of the game for a bit and also was still definitely learning before I left...so take my opinion for whatever it's worth and disregard anything off base!):

It seems like (maybe) one way to step through getting us in the right spot to start using Tailwind...if our goal is to ultimately actually make good use of Particle/Tailwind is:

  1. Upgrade to Tailwind 2.0. We aren't really using Tailwind much in the system yet (@stevendshelton may be an exception)...and the mag/custom styles are still reigning supreme. It seems unlikely this would cause any style issues since they indicate they have tried to make it backward compatible. How this works on the back-end/setup side...those are impacts I don't know about.

  2. Compare stylesheets. Compare the Tailwind 2.0 stylesheet (I'm not sure how to get to the full stylesheet?) to themag and CKeditor stylesheets (do we need to look at themaglegacy as well?) to see what styles they have in common (an idea for how to accomplish this here.)

We may do this and realize the differences are not great between the two. If this is the case, we could probably skip step 3 and 4 and just go ahead with the !important in Tailwind settings.

  1. From that comparison, identify any important differences in classes (I think the margin and padding classes are going to be the biggest offenders -- I did see use of these in elements like the buttons that are used across the website) that might impact styles across the site.

  2. Search for where problematic classes are in use and update them. Work on a PR that updates these items to use a class from Tailwind that will keep them looking similar to how they look now. (Basically, when we switch on !important and tailwind starts to override themag where the class names are the same, we want to be sure the defined tailwind style isn't wildly different from that of the mag class that was in use).

  3. Add the !important adjustment to the PR with the above changes.

OR....

  1. Update to Tailwind 2.0.
  2. Turn on !important so Tailwind overrides themag where classes have the same name.
  3. Fix issues as we see them.

So...one option is clearly shorter on the frontend, but feels scary to me with so many live pages on the site using elements that might be impacted. I might also be overthinking it and the differences aren't that great. That css comparison could help us know for sure.

If this is something I can help with, just lmk. I think it's up to ya'll on whether this has immediate value or not. I'm good either way. I know there are other items I can jump in on if we want to just say "not now" on this...I'll just keep doing custom styles, no problem. :)

@darkbluewalrus @UTCGilligan

bmartinez287 commented 3 years ago

https://tailwindui.com/changes-for-v2. We were not using tailwindui but the fact the free version absorbed some features its nice and good to know.

bmartinez287 commented 3 years ago

https://tailwindcss.com/docs/upgrading-to-v2

bmartinez287 commented 3 years ago

So, based on this

Setting important to true can introduce some issues when incorporating third-party JS libraries that add inline styles to your elements. In those cases, Tailwind's !important utilities defeat the inline styles, which can break your intended design.

We probably need to do a hybrid. Or else we will be stepping on digital measures, bridge edge and google search and any other third-party scripts.

bmartinez287 commented 3 years ago

Screen Shot 2021-02-08 at 2 11 06 PM So luckily for us most of the styles load fine. This is the only one template i found that triggered a change. I just fixed this in the PR below

bmartinez287 commented 3 years ago

Relates #174

brittrich commented 3 years ago

Looking good! I think I'm tracking, you've walked through the Tailwind installation steps to make sure we are updating any classes that will change in the shift to 2.0.

I took a look at a few pages to see impacts from the changes in how margin/padding are defined.

Example (slightly less margin between button sets across site):

Screen Shot 2021-02-09 at 7 10 12 AM
Comparison of themag and tailwind numbering of m- and p- classes: Number of padding/margin Tailwind theMag
0 0rem 0rem
0.5 0.125rem n/a
1 0.25rem 0.25rem
1.5 0.375rem n/a
2 0.5rem; 0.5rem
2.5 0.625rem n/a
**3 margin: 0.75rem 1rem**
3.5 0.875rem n/a
**4 1rem 1.5rem**
**5 1.25rem 2rem**
**6 1.5rem 3rem**
**7 1.75rem 4rem**
**8 2rem 6rem**

Tailwind scales up slowly all the way to 24rem (m-96).

I know this may have impacts on:

Ultimate Q: Is it worth updating the margin on the blocks? I think it will be an update to the field settings

brittrich commented 3 years ago

Another thing I noticed, is this a new issue when this change is enabled? I'll compare once I can get into cloudprod.

Screen Shot 2021-02-09 at 7 56 41 AM
bmartinez287 commented 3 years ago

Screen Shot 2021-02-09 at 3 06 43 PM This web tool is the closest I found to something that compares the differences. That said, due to boostrap and tailwincss mix in there is a rather large file. http://www.alanhart.co.uk/tools/compare-css.php

brittrich commented 3 years ago

So, based on this

Setting important to true can introduce some issues when incorporating third-party JS libraries that add inline styles to your elements. In those cases, Tailwind's !important utilities defeat the inline styles, which can break your intended design.

We probably need to do a hybrid. Or else we will be stepping on digital measures, bridge edge and google search and any other third-party scripts.

I have a question about this, and maybe the entire !important concept. Is Particle a subtheme whose base is themag? If so, shouldn't Particle classes be overriding themag? Is the fix, possibly, not in using !important but in tweaking some order of import, or process that happens in compiling of the css? The info.yml and libraries.yml for Particle seem like they are set up correctly, so not sure where the issue would lie. If this line of thought actually leads anywhere productive, maybe it could save us from this conflict with inline styles. I'm probably just being dense here, but at least thought I'd ask. :)

brittrich commented 3 years ago

Screen Shot 2021-02-09 at 3 06 43 PM This web tool is the closest I found to something that compares the differences. That said, due to boostrap and tailwincss mix in there is a rather large file. http://www.alanhart.co.uk/tools/compare-css.php

This is a really interesting comparison

Screen Shot 2021-02-09 at 3 06 43 PM This web tool is the closest I found to something that compares the differences. That said, due to boostrap and tailwincss mix in there is a rather large file. http://www.alanhart.co.uk/tools/compare-css.php

So, this is a mega huge file for sure. I can already see a few interesting differences that might have an impact (e.g. a:hover has no underline in tailwind), but it seems like on a scroll that most classes are not overlapping. What do you think? Worth looking through for things that will be different when tailwind takes precedence? I could run some excel macros to speed up deleting the unused classes if that's something we want to do.

brittrich commented 3 years ago

Okay, I was curious, so I did unrolled the css to make it as straight as possible (e.g. no class groupings) and compared the app.css and themag.css using the alanhart tool. Then, I used a macro to remove any classes that weren't in both. Aside from waiting for the macro to run, not too much time. An interesting process that we might need later if/when we ever want to get themag out of the project.

Here's what was left (margin/padding didn't come in because of the sm/ md/ that particle puts in front of those, so we need to remember those have some differences as well, see above comment): https://docs.google.com/spreadsheets/d/1kavfmLGXhm17fUt2HKQ2HkgQ_a7hDJFhlLmwqDr9b1I/edit?usp=sharing

Ultimately, looks like impacts will be minimal when particle takes precedence (of course, very possible I missed something). See the PR where I added further thoughts on this (https://github.com/UTCWeb/particle/pull/174#pullrequestreview-588527846).

bmartinez287 commented 3 years ago

So, based on this

Setting important to true can introduce some issues when incorporating third-party JS libraries that add inline styles to your elements. In those cases, Tailwind's !important utilities defeat the inline styles, which can break your intended design.

We probably need to do a hybrid. Or else we will be stepping on digital measures, bridge edge and google search and any other third-party scripts.

I have a question about this, and maybe the entire !important concept. Is Particle a subtheme whose base is themag? If so, shouldn't Particle classes be overriding themag? Is the fix, possibly, not in using !important but in tweaking some order of import, or process that happens in compiling of the css? The info.yml and libraries.yml for Particle seem like they are set up correctly, so not sure where the issue would lie. If this line of thought actually leads anywhere productive, maybe it could save us from this conflict with inline styles. I'm probably just being dense here, but at least thought I'd ask. :)

So idk the answer to this one. What you saying makes sense, thats how Drupal theming is supposed to work. Maybe there's a setting to add more weight to particle classes i gotta check on that. Great thoughts. If we could avoid !important that be awesome as people have complain it overrides styles coming from thirdparty scripts which could be problematic.

bmartinez287 commented 3 years ago

I was looking at that tool yesterday. However, I did not make such an in-depth study with it so this is awesome. I will take a look over the data and provide feedback on that tomorrow. Throughout my testing on that PR, the margin and padding changes are probably there but they have little effect on the visual aspect. That's not to say we should not study them careful but at least they are non critical.

bmartinez287 commented 3 years ago

TheMag parent has the following library settings. Screen Shot 2021-02-12 at 2 36 20 PM

TheMag Child has the following settings Screen Shot 2021-02-12 at 2 37 56 PM

Particle has the following. Screen Shot 2021-02-12 at 2 38 35 PM

https://www.drupal.org/docs/develop/standards/css/css-file-organization-for-drupal-8 https://www.drupal.org/forum/support/module-development-and-code-questions/2019-07-31/control-load-order-of-custom-module https://www.drupal.org/docs/theming-drupal/adding-stylesheets-css-and-javascript-js-to-a-drupal-theme https://www.drupal.org/docs/8/modules/libraries-api-8x/library-definition-structure

Based on that, I think we can increase the weight and make tailwindcss take priority. That said, I have too look for some class examples to test that theory on.

bmartinez287 commented 3 years ago

I probably leave !important for now. I just notice while looking at your example above that's is not an issue on the theme hierarchy but an issue with themag parent using !important overrides. /gary-w-rollins-college-of-business

bmartinez287 commented 3 years ago

Screen Shot 2021-02-16 at 4 50 26 PM I gotta update the colors and those icons for reason they seem to be missing.

brittrich commented 3 years ago

I probably leave !important for now. I just notice while looking at your example above that's is not an issue on the theme hierarchy but an issue with themag parent using !important overrides. /gary-w-rollins-college-of-business

Ah. That makes sense!

brittrich commented 3 years ago

@bmartinez287 another thing I noticed while working on the department directory styles (I implemented !important for particle on that branch so I could use the styles) is that drupal doesn't like the md: or lg: (or whatever) to be used in a view field class (in config). It removes the colon. A bit annoying since those responsive modifiers in tailwind would be very handy to use in some spots in the directory table. This issue has a solution, but I'm not savvy enough to know if it's one that would work for us: https://www.drupal.org/project/drupal/issues/3050007

bmartinez287 commented 3 years ago

if you could add a code sample and a few steps to reproduce that exact scenario that would be awesome. By config you mean twig or yml files? Twig files should be fine with it, yml files may not but we should not rely on those at least for now.

bmartinez287 commented 3 years ago

So, after doing some research and reading over that issue it appears they are planning to include those at some point however, the : character is not in the pipeline yet. That's mostly due to them being scape characters. This primarily would affect in place editing of views so if it can be avoided that be ideal.

brittrich commented 3 years ago

So, after doing some research and reading over that issue it appears they are planning to include those at some point however, the : character is not in the pipeline yet. That's mostly due to them being scape characters. This primarily would affect in place editing of views so if it can be avoided that be ideal.

Okay, disregard for now, I think we can see if it's a barrier to things as we go. I think there may be a way for me to work around it, possibly, in the dept directory styling...but it might require pulling in twig variables more granularly in the template. Everything is a tradeoff in Drupal...and there are only 500 ways to get to the same endpoint. ;)