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

Converted all the css into Tailwind css of Badge component #1739

Conversation

subhajit20
Copy link
Contributor

Description of changes

Converted all the css into Tailwind css of Badge component
Issue - #1725

Issue Resolved

Fixes #NA

Screenshots/GIFs

@vercel
Copy link

vercel bot commented Sep 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
operation-code ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 20, 2023 1:48pm
storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 20, 2023 1:48pm

@cypress
Copy link

cypress bot commented Sep 19, 2023

2 flaky tests on run #4442 ↗︎

0 35 0 0 Flakiness 2

Details:

resolve styling problems
Project: operation_code Commit: d371d872a8
Status: Passed Duration: 02:45 💡
Started: Sep 20, 2023 1:49 PM Ended: Sep 20, 2023 1:52 PM
Flakiness  hashlink.spec.js • 1 flaky test • all tests

View Output Video

Test Artifacts
Hash Links > on About page > will change route when clicked Output Screenshots Video
Flakiness  modal.spec.js • 1 flaky test • all tests

View Output Video

Test Artifacts
when the server responds successfully > closes the modal when the x button is clicked Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Member

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

almost done here

components/Badge/Badge.module.css Outdated Show resolved Hide resolved
components/Badge/Badge.js Outdated Show resolved Hide resolved
@kylemh
Copy link
Member

kylemh commented Sep 19, 2023

After you finish this PR, perhaps you could integrate Tailwind into our Storybook instance? I forgot about the need to do that!

Co-authored-by: Kyle Holmberg <inbox@kylemh.com>
components/Badge/Badge.js Outdated Show resolved Hide resolved
@subhajit20
Copy link
Contributor Author

After you finish this PR, perhaps you could integrate Tailwind into our Storybook instance? I forgot about the need to do that!

Okay cool, in the same way I have to do that in stodybook components right?

@kylemh
Copy link
Member

kylemh commented Sep 19, 2023

There shouldn't be any CSS modules to convert in Storybook, it's just adjusting the Storybook config so Tailwind is part of its build process. Right now, storybook has no idea what m-1 does, for example.

@subhajit20
Copy link
Contributor Author

There shouldn't be any CSS modules to convert in Storybook, it's just adjusting the Storybook config so Tailwind is part of its build process. Right now, storybook has no idea what m-1 does, for example.

Okay got it.
Would you merge this pr?

@kylemh
Copy link
Member

kylemh commented Sep 19, 2023

I will as soon as I can! I need to resolve an issue that's visible on this PR's preview deployments on the /services route with the Badge components in-use. If I merge without resolving the issue, the problem goes live to prod!

There's nothing you need to do in this PR more though!

@subhajit20
Copy link
Contributor Author

I will as soon as I can! I need to resolve an issue that's visible on this PR's preview deployments on the /services route with the Badge components in-use. If I merge without resolving the issue, the problem goes live to prod!

There's nothing you need to do in this PR more though!

Alright!

@codeclimate
Copy link

codeclimate bot commented Sep 20, 2023

Code Climate has analyzed commit d371d87 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 75.8% (0.0% change).

View more on Code Climate.

@kylemh
Copy link
Member

kylemh commented Sep 20, 2023

So, @subhajit20 I noticed a problem that you'll need to be aware of...

We've defined the element with the ID __next as a point for tailwind to raise its specificity from (source) (tailwind docs on the topic).

The downside to this approach is that we need to fixes like this to all the places USING a component we do a Tailwind update to. It's lame, but at least it's formulaic and simple.

We could not do important: '#__next' in the Tailwind config; however, if we do that we have the same problem in the other direction... and the solution seems much worse: using the important selector for all Tailwind classes we want to "win".

The benefit of changing the CSS modules files is that it's temporary, since our end goal has us deleting CSS modules files and the "winner" going forward will just be whatever classname was applied last (ideal for us).

Anyways... to look out for this, you'll want to leverage the "find all references" action in VS Code:

Screenshot 2023-09-20 at 4 55 17 PM

For every component that you work on, you'll want to look for usages, find the page they're rendered on, look at the page and compare the same UI to prod, and either update Tailwind all the way through to the implementation level OR do the fix I recommended above.

@kylemh kylemh merged commit ab91e08 into OperationCode:main Sep 20, 2023
3 checks passed
@subhajit20
Copy link
Contributor Author

So, @subhajit20 I noticed a problem that you'll need to be aware of...

We've defined the element with the ID __next as a point for tailwind to raise its specificity from (source) (tailwind docs on the topic).

The downside to this approach is that we need to fixes like this to all the places USING a component we do a Tailwind update to. It's lame, but at least it's formulaic and simple.

We could not do important: '#__next' in the Tailwind config; however, if we do that we have the same problem in the other direction... and the solution seems much worse: using the important selector for all Tailwind classes we want to "win".

The benefit of changing the CSS modules files is that it's temporary, since our end goal has us deleting CSS modules files and the "winner" going forward will just be whatever classname was applied last (ideal for us).

Anyways... to look out for this, you'll want to leverage the "find all references" action in VS Code:

Screenshot 2023-09-20 at 4 55 17 PM

For every component that you work on, you'll want to look for usages, find the page they're rendered on, look at the page and compare the same UI to prod, and either update Tailwind all the way through to the implementation level OR do the fix I recommended above.

Okay will check it

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.

2 participants