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.

Sign up for GitHub

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

Jump to bottom

feat(cxl-lumo-styles): increase default font size for p elements #423

Closed
wants to merge 1 commit into from
Closed

feat(cxl-lumo-styles): increase default font size for p elements #423

wants to merge 1 commit into from

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented Jul 11, 2024

@anoblet anoblet requested review from lkraav and pawelkmpt July 11, 2024 12:51
@anoblet anoblet mentioned this pull request Jul 11, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 11, 2024 •
edited
Loading

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 44.32 KB (+0.2% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 29.02 KB (+0.1% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 138.23 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-institute.js, 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 276.05 KB (+0.08% 🔺)

pawelkmpt
pawelkmpt requested changes Jul 19, 2024
View reviewed changes
Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

I have trouble finding where actually anything has changed. I looked at Storybook in typography, layout and these pages look the same.

Also take into account block of text could have ul, ol and similar tags too and their size need to be the same as p

packages/cxl-lumo-styles/scss/typography.scss Outdated
Comment on lines 113 to 118

/**
* Improve readability.
*
* @since 2024.07.09
*/
Copy link

Choose a reason for hiding this comment

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

Comment does not provide any value

Copy link

@pawelkmpt pawelkmpt Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean, just remove it. It's just a font size. You can add the link to the commit message though

@anoblet anoblet force-pushed the anoblet/feat/typography branch from 520af5d to 0c7064f Compare July 24, 2024 14:07
@anoblet anoblet force-pushed the anoblet/feat/typography branch from 0c7064f to 9d0351e Compare July 24, 2024 14:14
@pawelkmpt
Copy link

pawelkmpt commented Jul 25, 2024 •
edited
Loading

I looked at Storybook, compared master and this branch. It looks like bigger font size has also impact on layout width in some places. It affects (some) cards too.

Please experiment with such styles on the live site and make sure things won't be broken. Especially landing pages and lessons.

Make screenshots and show us the difference.

BEFORE
1-before

AFTER
1-after

BEFORE
2-before

AFTER
2-after

BEFORE
3-before

AFTER
3-after

BEFORE
4-before

AFTER
4-after

@anoblet
Copy link
Collaborator Author

anoblet commented Jul 25, 2024

I believe you were correct in suggesting upgrading individual parts would be a better strategy. I updated the PR to increase the font size for .archive-description, .entry-content, .entry-summary.

Instead of merging this quickly, I'll update the live site with one or two rules a day to see if anything breaks. Once we can confirm everything looks good, I'll add it to this PR.

@pawelkmpt
Copy link

PR for blog is here: #430

@pawelkmpt pawelkmpt closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawelkmpt pawelkmpt pawelkmpt requested changes

@lkraav lkraav Awaiting requested review from lkraav

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anoblet @pawelkmpt