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

chore(debt): Move all static style values to classes #25035

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

anirudh24seven
Copy link
Contributor

Problem

Fixes #16826 by replacing all static styles with their class equivalents.

How did you test this code?

I manually verified some changes but I am not able to get pnpm test:visual working correctly on my system. Will share the results once I do. I did not want it to be a blocker for pushing the code changes that I have done.

@Twixes Twixes requested a review from a team September 18, 2024 09:42
}}
>
{onClose ? (
<div
onClick={onClose}
className="absolute origin-top-left bg-black text-white rounded-full w-6 h-6 z-[7] flex items-center justify-around text-center cursor-pointer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we have a color var black defined in the $colors list of vars.scss – probably should be:

Suggested change
className="absolute origin-top-left bg-black text-white rounded-full w-6 h-6 z-[7] flex items-center justify-around text-center cursor-pointer"
className="absolute origin-top-left bg-[#000] text-white rounded-full w-6 h-6 z-[7] flex items-center justify-around text-center cursor-pointer"

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Just one snag, but overall solid refactor!

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.

Replace style prop with utility css classes
3 participants