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: created plugin to prefix class names to prevent styling conflicts #235

Conversation

marcusgchan
Copy link
Contributor

@marcusgchan marcusgchan commented Aug 1, 2023

Description

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes #89

Mobile & Desktop Screenshots/Recordings

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@marcusgchan
Copy link
Contributor Author

Can I get some suggestions @diivi @Anush008 @bdougie? There are 4 points I would like to discuss.

  1. The plugin will prefix every single className in the jsx. For example,
    <div className="flex gap-2 random-class"></div> will become:
    <div className="tw-flex tw-gap-2 tw-random-class"></div>
    which will mess up additional non tailwind classes unless we prefix those too.
  2. It also won't prefix classes if the classes are created with function calls:
    <div className={`flex gap-2 random-class ${getClasses()}`}"></div>
  3. It won't prefix classes added with the ternary operator but i think this is doable if i modify the code a bit. However, if we don't need the functionality now then I'm not sure if I should implement it<div className={`flex gap-2 random-class ${foo ? "bg-white" : "bg-blue"`}></div>
  4. It won't work with common css utility libraries like clsx

There are probably other cases too but this is what came to my mind while working on it so far.

@nickytonline
Copy link
Member

I don't have the context for this PR and I don't see an issue linked to this, so I'm curious why we need to prefix things to avoid styling conflicts? Tailwind classes wouldn't have conflicts, but if non-Tailwind ones did, you could use CSS modules or a similar styling solution. But if we're using Tailwind, I'm a bit confused as to why this is an issue?

@diivi
Copy link
Contributor

diivi commented Aug 3, 2023

Please don't leave the "Related Issues/Documents" field empty πŸ˜…. Here's the issue btw - #89

@bdougie
Copy link
Member

bdougie commented Aug 3, 2023

  1. The plugin will prefix every single className in the jsx. For example,

This is fine for me especially if this is happening in production only. I'd suggestion a prefix oss

<div className="flex gap-2 random-class"></div> will become:

<div className="tw-flex tw-gap-2 tw-random-class"></div>

which will mess up additional non tailwind classes unless we prefix those too.

This is fine, we are just trying to avoid collisions with github. So if we cover most classes, we should be fine, but we won't know until we implement it and QA

  1. It also won't prefix classes if the classes are created with function calls:

<div className={`flex gap-2 random-class ${getClasses()}`}"></div>

Same with above, let's see if this solves the case in the notifications #89 panel and go on from there

  1. It won't prefix classes added with the ternary operator but i think this is doable if i modify the code a bit. However, if we don't need the functionality now then I'm not sure if I should implement it<div className={`flex gap-2 random-class ${foo ? "bg-white" : "bg-blue"`}></div>

Same as above

  1. It won't work with common css utility libraries like clsx

Same

There are probably other cases too but this is what came to my mind while working on it so far.

No need to solve all cases since the scope is the one case that has been reported

@marcusgchan
Copy link
Contributor Author

Sounds good. How about content scripts since those have a mixture of GitHub and tailwind classes? How would we differentiate between them?

@marcusgchan
Copy link
Contributor Author

There are 2 issues that I've ran into @bdougie @diivi @Anush008.

  1. How to handle classes for content scripts since those have a mixture of Github and tailwind classes. One option could be to prefix those manually.
  2. PostCSS (tailwind plugin) runs before the vite plugin to prefix classes. This is an issue since tailwind only adds classes that are used so currently no tailwind classes are added. For example, adding the prefix option in the tailwind config will give a warning "No utility classes were detected in your source files. If this is unexpected, double-check the content option in your Tailwind CSS configuration.". Afterwards, the vite plugin will run to prefix the classes.

@bdougie
Copy link
Member

bdougie commented Aug 10, 2023

There are 2 issues that I've ran into @bdougie @diivi @Anush008.

  1. How to handle classes for content scripts since those have a mixture of Github and tailwind classes. One option could be to prefix those manually.
  2. PostCSS (tailwind plugin) runs before the vite plugin to prefix classes. This is an issue since tailwind only adds classes that are used so currently no tailwind classes are added. For example, adding the prefix option in the tailwind config will give a warning "No utility classes were detected in your source files. If this is unexpected, double-check the content option in your Tailwind CSS configuration.". Afterwards, the vite plugin will run to prefix the classes.

We started this exploration due to some specific conflicts. Is it possible to prefix manually only in areas we are aware of the conflicts? i.e. #89 (comment)

@marcusgchan
Copy link
Contributor Author

@bdougie Another option we could explore is having multiple tailwind configs since we won't have to deal with the edge cases with prefixing every tailwind class and we would have to prefix the content scripts manually anyways. So the popup could have the same tailwind config and the content scripts could have another tailwind config with the prefix set.

@marcusgchan marcusgchan force-pushed the fix/prefix-classnames-to-prevent-styling-conflicts branch from 2e529d2 to 7db291e Compare August 14, 2023 09:02
@bdougie bdougie closed this Sep 20, 2023
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.

Bug: change in size GitHub (UX)
4 participants