-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Convert cursor.module.css
to CSS modules
#40522
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 359a8da...9e06333.
|
|
CS.inlineBlock, | ||
CS.full, | ||
CS.cursorPointer, | ||
`text-${color}-hover`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be hard to find when we will need to migrate such classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankfully that's the only class name I can find that's dynamically generated. I've seen it a billion times and should be sorted with colors.module
.
And, of course as I look at it I didn't sort it in the colors module. I'll do that there
@@ -1,15 +1,11 @@ | |||
:global(.cursor-pointer), | |||
.cursor-pointer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, didn't see the second one. I know the e2e test needs to be sorted but I'll do that in a sec
@@ -267,7 +267,7 @@ function TargetWithSource({ | |||
</div> | |||
</div> | |||
<div | |||
className="cursor-pointer ml-auto" | |||
className={cx(CS.cursorPointer, CS.mlAuto)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mlAuto
needs to be added along .ml-auto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I am getting caught doing all of these PRs. I'll stack them in the future
0366e9d
to
9a060d9
Compare
onMove={(source, destination) => | ||
dispatch(Collections.actions.setCollection(source, destination)) | ||
} | ||
onMove={async (source, destination) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are not related to the PR, most likely there were some issues with merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh what. Ok I'll reset to the last good commit and go from there
15be1ee
to
6cb2621
Compare
@oisincoveney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Description
Describe the overall approach and the problem being solved.
How to verify
Describe the steps to verify that the changes are working as expected.
Demo
Upload a demo video or before/after screenshots if sensible or remove the section
Checklist