PWA-1841 & PWA-2302: Convert CSS to Tailwind#3686
Conversation
jimbo
added
the
version: Major
|
|
@jimbo Heads up, Prettier broke all the arbitrary values in Tailwind that contain spaces. TW specifies these have to be written without spaces to work. Example:
|
jimbo
force-pushed
the
brendan/PWA-1895-monarch
branch
from
de05be7 to
38fbbc9
Compare
|
Might an underscore work? |
|
@fooman That should solve the conflict Prettier causes after we update to TW3. This build is still on TW2 because the work started before TW3 was released. It's probably why they changed from Under TW2 using
Temporarily, we'll have to wrap the broken declarations with Added a PR into this PR to patch this temporarily: #3704 |
…ier-workaround PWA-1895: workaround Prettier mangling arbitrary values in Tailwind
| @@ -1,10 +1,10 @@ | |||
| { | |||
| "name": "@magento/pwa-theme-venia", | |||
| "name": "@magento/pwa-theme-base", | |||
There was a problem hiding this comment.
Should we hold off renaming the package until the next release? Technically, this is still the Venia theme.
m2-community-project
bot
added
Progress: review
and removed
Progress: work in progress
labels
|
What's the reason behind composing Tailwind classes from the global namespace instead of embedding them directly in HTML tags? From first glance, it looks like there's a lot of repetition so I'm curious as to what's the reason causing this to be less-important. Thanks. |
| line-height: 1; | ||
| pointer-events: auto; | ||
| text-align: center; | ||
| composes: items-center from global; |
There was a problem hiding this comment.
this should be sorted below inline-flex
|
@drpayyne Using Tailwind inside CSS Modules isn't a perfect solution (it causes some specific issues), but it solves most of the problems this solution was architected for: support theming without changing JSX or requiring TW adoption for backward compatibility. There is a lot of repetition when authoring, but it doesn't affect the build size so the benefits of Tailwind aren't lost. The classes written into JSX components are essentially a static API. Existing implementations aren't required to use Tailwind, and if all JSX changed we'd be forcing them to migrate all their custom CSS into TW as most CSS imports would be removed. Additionally, extensions can use the "targets" functionality on CSS without having affecting JSX, which reduces the long-term maintenance scope for implementations. |
jcalcaben
left a comment
jcalcaben
left a comment
There was a problem hiding this comment.
Some initial issues I noticed while clicking through the storefront and comparing
| @@ -24,14 +26,15 @@ | |||
|
|
|||
| .submenu_active { | |||
There was a problem hiding this comment.
currently, the mega menu stays hidden even on hover in this PR
we should add a definition for the inactive state:
.submenu_inactive {
composes: submenu;
composes: hidden from global;
}
and make the necassary changes to submenu.js
There was a problem hiding this comment.
Fixed. This is a general change in pattern, too.
Currently, we often make one state the default and use composition to override properties for the remaining states; the loader inserts module rulesets in order, so later rulesets take precedence over earlier ones and everything works as expected.
With Tailwind, the order of rulesets is predetermined for the global stylesheet. When everything is composing from these global rules, ruleset order within a module makes no difference. This means we need an individual ruleset for each state; in this case, it was submenu_inactive.
| .root:focus { | ||
| box-shadow: -6px 6px rgb(var(--venia-brand-color-1-100)); | ||
| composes: disabled_border-gray-400 from global; | ||
| composes: disabled_pointer-events-none from global; |
There was a problem hiding this comment.
Button disabled state looks off with grey border and text on a blue background.
Shouldn't it be white border and text on a grey background?
| borderColor: theme => ({ | ||
| button: theme('colors.gray.600'), | ||
| error: theme('colors.red.400'), | ||
| info: theme('colors.emerald.600'), |
There was a problem hiding this comment.
colors.emerald doesn't seem to work even though it's a default entry in the Tailwind Color Palette.
you can test this out by click on an Add to Favorites link and looking at the toast. It should have a green-ish border, but it is gray.
Weirdly enough, colors.green works.
There was a problem hiding this comment.
Yeah, they fixed this in Tailwind 3, which we aren't using yet. Fixed.
| } | ||
|
|
||
| .controls { | ||
| composes: border from global; |
m2-community-project
bot
added
Progress: changes requested
and removed
Progress: review
labels
| } | ||
|
|
||
| .link { | ||
| composes: root from '../LinkButton/linkButton.module.css'; |
There was a problem hiding this comment.
Should be refactored to remove the cross-component dependency.
This currently renders as a center justified list of blue links instead of the left justified list of grey items
There was a problem hiding this comment.
Fixed.
Also, I agree. We should get rid of all cross-component dependencies, since Tailwind handles deduplication for us now automatically. If we really need some shared CSS, we can write a Tailwind plugin.
| } | ||
| .addButton { | ||
| composes: border-2 from global; | ||
| composes: border-solid from global; |
| .dropdown { | ||
| align-items: center; | ||
| composes: absolute from global; | ||
| composes: bg-xxx from global; |
There was a problem hiding this comment.
I forgot to mention this entry is also missing composes: bg-white from global;
Right now it's rendering a clear background.
| button: '20', | ||
| buttonHover: '21', | ||
| buttonFocus: '22', | ||
| dropdown: '50', |
There was a problem hiding this comment.
we may need to split this into headerDropDown and contentDropDown.
currently, this is being used in the dropdown module in the Legacy Mini Cart and it is causing the modal to render on top of the header when you scroll down
There was a problem hiding this comment.
Fixed. Let me know if you spot any issues as a result of my change here—seems good to me.
m2-community-project
bot
added
Progress: review
and removed
Progress: changes requested
labels
|
Successfully started codebuild job for |
|
run cypress |
|
Successfully started codebuild job for |
|
run cypress |
|
Successfully started codebuild job for |
|
run cypress |
|
Successfully started codebuild job for |
|
run cypress |
|
Successfully started codebuild job for |
|
run lighthouse-desktop |
|
Successfully started codebuild job for |
|
run lighthouse-mobile |
|
Successfully started codebuild job for |
|
run lighthouse-mobile |
|
Successfully started codebuild job for |
|
run lighthouse-mobile |
|
Successfully started codebuild job for |
|
QA Approved. |
|
🎉🎉🎉 |
|
@jimbo @jcalcaben I apologize for writing in a closed pr, but tell me, why do we use composes from the global space to write css through css modules with tailwind, except for writing styles through the apply utility? in this case, we lose autocomplete, and incorrect classes are ignored and simply not applied without errors. use this: |
9 participants
Description
Convert as many CSS rules to Tailwind (via
composes) as possible.Related Issue
Closes PWA-1841 & PWA-2302.
Acceptance
Verification Stakeholders
Specification
Verification Steps
Test scenario(s) for direct fix/feature
Full regression needed; some discrepancies are acceptable within reason.
Test scenario(s) for any existing impacted features/areas
Test scenario(s) for any Magento Backend Supported Configurations
--
Is Browser/Device testing needed?
Yes; all components are affected across all devices.
Any ad-hoc/edge case scenarios that need to be considered?
TBD.
Screenshots / Screen Captures (if appropriate)
Breaking Changes (if any)
Components TBD. All styles are changing.
Checklist