Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(cxl-ui): fix mobile layout issues in course pages #377

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

freudFlintstone
Copy link

No description provided.

Copy link

github-actions bot commented Dec 19, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 69.68 KB (-0.05% 🔽)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.83 KB (-0.12% 🔽)
packages/cxl-ui/pkg/dist-web/vendor.js 140.5 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 252.05 KB (-0.03% 🔽)

@freudFlintstone freudFlintstone force-pushed the raphael/fix/navigation/mobile branch 2 times, most recently from 7c95394 to b5a9f38 Compare December 22, 2023 20:15
@freudFlintstone freudFlintstone self-assigned this Dec 22, 2023
@freudFlintstone
Copy link
Author

Task linked: CU-86aykudx5 Deployment of new component

@freudFlintstone freudFlintstone marked this pull request as ready for review December 22, 2023 20:16
@anoblet
Copy link
Collaborator

anoblet commented Dec 26, 2023

This looks good! It seems you have a better idea of the issue and could have it solved quicker. Should we revert this commit? 73a306b

@freudFlintstone
Copy link
Author

freudFlintstone commented Dec 27, 2023

This looks good! It seems you have a better idea of the issue and could have it solved quicker. Should we revert this commit? 73a306b

@anoblet I definitely wish I had understood the issue faster. But this is not a proper fix, it's just a workaround, since we still don't know what's causing the overflow.

Do not revert the other commit. It fixes a real issue that would have the same effect. The reason I thought it was the only cause is that it fixed it in local development.

Copy link
Collaborator

@anoblet anoblet left a comment

Choose a reason for hiding this comment

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

This should fix the menu gap issue. Adding overflow hidden to cxl-app-layout works using inspector on live. There's also a chance it could fix some of the animation issues we face when opening the menu for the first time.

@pawelkmpt pawelkmpt force-pushed the raphael/fix/navigation/mobile branch from b5a9f38 to 55de700 Compare January 3, 2024 10:50
@pawelkmpt
Copy link

@anoblet I tested the code on live, fixed one issue @freudFlintstone introduced (missing card styles due the removal of @import clause).

I also deleted b5a9f38 with overflow: hidden for cxl-app-layout as it's just hiding an issue, not solving it and also can cause problems in other places.

This code is live on blog and institute.

@pawelkmpt pawelkmpt requested a review from anoblet January 3, 2024 10:53
@anoblet
Copy link
Collaborator

anoblet commented Jan 3, 2024

This seems to fix https://cxl.com/webinars. On Institute pages we still have a horizontal scroll which I can't seem to find in inspector.

@pawelkmpt pawelkmpt merged commit 4cf93a8 into master Jan 4, 2024
5 checks passed
@pawelkmpt pawelkmpt deleted the raphael/fix/navigation/mobile branch January 4, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants