Skip to content
New issue

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

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

Already on GitHub? # to your account

Tailwind v3 Update #4082

Conversation

justinconabree
Copy link
Contributor

@justinconabree justinconabree commented Apr 11, 2023

Description

  • Updating Tailwind dependency to v3 (latest). Some required fixes documented here
  • Fixing priority issues caused by Tailwind update. These were already described as B6 issues, but the update exacerbated the problem. The fix for this was using @apply when using composes from another class and overriding values.
    • Note styles may change from this as some styling isn't currently applied due to the B6 issue in current Venia (small bugs throughout the site previous to this change). In some cases I've removed the overrides so existing styles stay consistent.
  • Changing tokens and index css files to use tailwind configurations
  • Exposing key colors as css variables for use in legacy tokens
  • Adding new values to theme config and replacing usage in css files
  • Using new button and input border roundness for better configurability
  • Changing media queries to tailwind variants
  • Fixing many @TODO tailwind comments

Styles that have changed due to B6 being corrected

  • Footer where the columns are spaced evenly
  • Checkout form area when editing no longer has border

Related Issue

Closes #4081

Acceptance

Verification Stakeholders

Specification

Changes to theme must be reviewed

Verification Steps

Test scenario(s) for direct fix/feature

Full pass through site to verify no major style changes across all screen sizes

Test scenario(s) for any Magento Backend Supported Configurations

None

Is Browser/Device testing needed?

Yes, device testing is required as styles may vary based through all breakpoints

Any ad-hoc/edge case scenarios that need to be considered?

All features (states) must be tested

Breaking Changes (if any)

Updating PWA Studio (Venia) from Tailwind V2 to V3 has required some changes.

For starters, developers should look over Tailwind's Upgrade Guide to see what the general implications of Tailwind V3 are for their project.

Some additional configurations have been made available in the theme and implemented in Venia. These include

  • New border radiuses for component types
    • input, button, box, badge
  • Border colors
    • input
    • swatch
    • base
    • current color (fix for TW3)
  • Line heights attached to font size
  • Additional media queries (max screen sizes, min/max height)
  • Shimmer
    • Background color
    • Animation
  • Spacing utils for screen widths and header spacing
  • Width utils
  • Screen sizes necessary to convert venia-ui to use Tailwind config in all instances (pagebuilder excluded)
    • Change xl screen size to 1024px (unused previous to this update)

Some key colors are also now exposed through CSS variables using the root plugin. These variables are then used in the base tokens file so legacy use now runs off of tailwind configuration.

Checklist

  • [ ] I have added tests to cover my changes, if necessary.
  • [ ] I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented Apr 11, 2023

Warnings
⚠️ Found the word "TODO" in the PR description. Just letting you know incase you forgot :)
Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next pr-test build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 8211c1e

@justinconabree
Copy link
Contributor Author

Hi @anthoula, @glo82145, can you please follow up with PWA Studio team to see if I should continue to work on this.

Thanks!

@glo82145
Copy link
Collaborator

@justinconabree Please feel free to proceed on this PR. Please assign this issue to yourself.

Thanks

@glo42707 glo42707 mentioned this pull request Jun 8, 2023
9 tasks
@justinconabree justinconabree marked this pull request as ready for review June 8, 2023 19:32
@justinconabree
Copy link
Contributor Author

justinconabree commented Jun 9, 2023

Hi @glo42707 ,

Work is done on my end. I've added some notes to the PR description that should be added to release notes eventually. There will be some implications for projects (major version bump necessary).

I'm seeing some issues in the deployed PR instance though. It isn't compiling @apply rules properly. They're coming out raw instead of compiled (eg @apply box-border VS box-sizing: border-box). This works locally with both yarn watch:venia and yarn build && yarn stage:venia. I've tested with both node 14 and 16. Any idea what might be happening?

Also there are some cypress tests failing, but they're also failing on develop so I haven't updated any of the snapshots.

@glo42707 glo42707 requested a review from supernova-at July 7, 2023 13:02
@glo42707
Copy link
Contributor

glo42707 commented Aug 7, 2023

hey @justinconabree

thanks for work on PR, now regarding your issue regarding size etc. that a knows issue due to one of active cypress comatibility. so it will be resolved in future, once after we will have fix available.

glo82145
glo82145 previously approved these changes Aug 7, 2023
@glo82145
Copy link
Collaborator

glo82145 commented Oct 4, 2023

run lighthouse-desktop

@pwa-test-bot
Copy link

pwa-test-bot bot commented Oct 4, 2023

Successfully started codebuild job for lighthouse-desktop

@glo82145
Copy link
Collaborator

glo82145 commented Oct 4, 2023

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Oct 4, 2023

Successfully started codebuild job for lighthouse-mobile

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented Oct 4, 2023

Fails
🚫 Lighthouse assertions failed 😔. All assertions must pass before this PR can be merged
https://pr-4082.pwa-venia.com/

categories.pwa (minScore)

  • Expected: 0.9
  • Actual: 0.89
  • Result: Failed

Max Potential First Input Delay (maxNumericValue)

  • Expected: 520
  • Actual: 552
  • Result: Failed
  • Docs link: https://developer.chrome.com/docs/lighthouse/performance/lighthouse-max-potential-fid/

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.3
  • Actual: 0.35496028252722606
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-4082.pwa-venia.com/venia-tops.html?page=1

categories.pwa (minScore)

  • Expected: 0.9
  • Actual: 0.89
  • Result: Failed

Max Potential First Input Delay (maxNumericValue)

  • Expected: 520
  • Actual: 552
  • Result: Failed
  • Docs link: https://developer.chrome.com/docs/lighthouse/performance/lighthouse-max-potential-fid/
https://pr-4082.pwa-venia.com/valeria-two-layer-tank.html

Max Potential First Input Delay (maxNumericValue)

  • Expected: 520
  • Actual: 554
  • Result: Failed
  • Docs link: https://developer.chrome.com/docs/lighthouse/performance/lighthouse-max-potential-fid/

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.9013345065691857
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-4082.pwa-venia.com/search.html?query=tops&page=1

categories.pwa (minScore)

  • Expected: 0.9
  • Actual: 0.89
  • Result: Failed

Max Potential First Input Delay (maxNumericValue)

  • Expected: 520
  • Actual: 551
  • Result: Failed
  • Docs link: https://developer.chrome.com/docs/lighthouse/performance/lighthouse-max-potential-fid/
🚫

node failed.

Log

ERROR ON TASK: lighthouseTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results-9e70d53a.json

Generated by 🚫 dangerJS against 332b5c9

@pwa-studio-bot
Copy link
Collaborator

Fails
🚫 Lighthouse assertions failed 😔. All assertions must pass before this PR can be merged
https://pr-4082.pwa-venia.com/

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.21876627099288753
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-4082.pwa-venia.com/venia-tops.html?page=1

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.30013716700571214
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-4082.pwa-venia.com/valeria-two-layer-tank.html

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.8567256355088054
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-4082.pwa-venia.com/search.html?query=tops&page=1

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.8666959306410258
  • Result: Failed
  • Docs link: https://web.dev/cls/

Max Potential First Input Delay (maxNumericValue)

  • Expected: 127
  • Actual: 138
  • Result: Failed
  • Docs link: https://developer.chrome.com/docs/lighthouse/performance/lighthouse-max-potential-fid/
🚫

node failed.

Log

ERROR ON TASK: lighthouseTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results-0b2f71a8.json

Generated by 🚫 dangerJS against a7b7178

@glo82145
Copy link
Collaborator

glo82145 commented Oct 4, 2023

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Oct 4, 2023

Successfully started codebuild job for lighthouse-mobile

@glo82145
Copy link
Collaborator

glo82145 commented Oct 4, 2023

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Oct 4, 2023

Successfully started codebuild job for lighthouse-mobile

@glo82145
Copy link
Collaborator

glo82145 commented Oct 4, 2023

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Oct 4, 2023

Successfully started codebuild job for lighthouse-mobile

@glo82145
Copy link
Collaborator

glo82145 commented Oct 6, 2023

@justinconabree Lighthouse tests are failing in this PR. Can you please look into that

@justinconabree
Copy link
Contributor Author

@glo82145 looking into it! Will provide updates early next week

@justinconabree
Copy link
Contributor Author

Hi @glo82145 ,

I'm seeing some weird behaviour. Locally with this branch the site looks okay compared to develop, but on the PR instance there's a lot of visual bugs. I've attached some screenshots.

local_instance
PR_instance

Any idea why this might be happening? I have a feeling at least some of these issues are behind the lower lighthouse scores

@glo42707
Copy link
Contributor

hey @justinconabree

those are default UI implamntation on PWA studio which is already in place since past few release as per my knowledge and it is not impacting in any other current PR pr PWA but only facing issue only with this PR 4082.
hence request to please check and fix for furthure step to merege.

@justinconabree
Copy link
Contributor Author

Hi @glo42707 ,

Is it possible the deployed PR instance is out of date with the branch? I made the PR when the work was incomplete. Does it get redeployed for each commit?

Thanks

@justinconabree
Copy link
Contributor Author

Hi @glo42707,

Just following up to see if you'd know why the PR instance doesn't reflect the up-to-date branch code. Is it possible to redeploy the code to the PR instance?

Thanks

@glo42707
Copy link
Contributor

glo42707 commented Nov 9, 2023

hey @anthoula and @supernova-at

could you please help on query asked by @justinconabree if that is possible case.

Thanks

@anthoula
Copy link
Contributor

anthoula commented Nov 9, 2023

It should redeploy after each new commit, and closing and opening a new PR as well.

@justinconabree
Copy link
Contributor Author

I'll try closing PR and opening a new one. pr-deploy task is marked as success but I'm still seeing major differences between local and PR instance. Hopefully new PR helps🤞

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:extensions pkg:pagebuilder pkg:pwa-theme-venia pkg:venia-concept pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature]: Tailwind v3 support
6 participants