search

open-sauced / ai

Discover open-source projects on GitHub relevant to your skills (with ai).
https://opensauced.ai
MIT License
72 stars 30 forks source link

Bug: change in size GitHub (UX) #89

Open a0m0rajab opened 1 year ago

a0m0rajab commented 1 year ago

Describe the bug

When you open GitHub with the extension enabled it resizes the screen and change dimension which is a bit uncomfortable.

Steps to reproduce

  1. enable the extension.
  2. go to: https://github.com/open-sauced/browser-extensions/issues/88
  3. you see a minor resize at the beginning, (you might need to force refresh to see the issue (CTRL+SHIFT+R))
  4. disable the extension
  5. go to link in step 2
  6. you see there is no resize

https://github.com/open-sauced/browser-extensions/assets/18273833/68245251-11a4-49e4-b077-07ee491b73d9

Affected services

browser-extension

Platforms

No response

Browsers

No response

Environment

No response

Additional context

No response

Code of Conduct

Contributing Docs

Nanak360 commented 1 year ago

Yes, I have noticed this issue as well. this is happening because of importing @tailwind utilities; in content-scripts.css

I also noticed issues on the issue page...hehe, get it? image

notice the issue button on the above image. It's not aligned properly. This happens when we have @tailwind utilities; imported

BUT if we remove @tailwind utilities; from the CSS sheet, the whole issue disappears but all the OpenSauced components look bad. Really bad. image

Maybe if we configure tailwind.config.js to match with github's tailwind config or something, we can solve this issue. Another solution can be defining our own CSS classes and using them.

a0m0rajab commented 1 year ago

Thank you for your comment! I think this might be a thing that @Anush008 or @diivi can discuss better than me. From my perspective, I think writing our own CSS class would be better and cause no conflict in the future. Even if anything changes on GitHub, it will be dependency free.

Nanak360 commented 1 year ago

writing our own CSS class would be better and cause no conflict in the future

I agree with you on this. Currently, there are 11 components being injected into GitHub, and it would indeed be relatively easy to modify them to use custom CSS classes. However, as the number of components grows, converting them all to custom CSS classes could become more challenging and time-consuming.

a0m0rajab commented 1 year ago

This Stackoverflow question might help, I quickly checked it but did not fully test the situation.

Nanak360 commented 1 year ago

This Stackoverflow question might help, I quickly checked it but did not fully test the situation.

This looks really promising

a0m0rajab commented 1 year ago

The solution I shared previously resolves the conflict, but since it's adding a prefix to tailwind, the react app gots broken. I researched further and found that we can configure react to add a prefix to classNames.

Some libraries are adding the prefix to className like this, and this one. Some people used to do it manually but I do prefer a library not to get out of the standard practices. Here is a related question in StackOverFLow and unanswered question in stackoverflow,

Another thing we can do is that since the project is using PostCSS we might add the prefix there.

A library to try, PostCSS library, postCSS prefixer, and PostCSS related StackOverFlow Question mentioning the library and showing an example of it.

I think the next step here would be trying both solutions,

  1. add prefix to tailwind
  2. add prefix to react classNames
  3. check the pure HTML (content scripts) result - this might be the tricky one -
marcusgchan commented 1 year ago

Another possible solution could be creating a shadow dom for each element that is inserted into the page since it will isolate styling from the host page.

marcusgchan commented 1 year ago

.take

Anush008 commented 1 year ago

Hey @marcusgchan, about the use of a shadow DOM.The injected elements inherit some of their styling from GitHub itself. The shadow DOM will obstruct this behaviour.

marcusgchan commented 1 year ago

I see, didn't realize that it was using some of the styling from GitHub

marcusgchan commented 1 year ago

I looked at the react-classname-prefix-loader and it seems interesting since it automates the process of adding prefixes during the build phase, but there are a few things that concern me:

  1. It hasn't been updated in 6 years
  2. There's a bug that incorrectly adds prefixes to attributes
  3. It adds a hidden abstraction where all classes will be prefixed unless added to the blacklist
  4. I'm not sure if there's a simple way to detect if a class should have a prefix for content scripts since it uses a mixture of github and tailwind classes

It may be easier to manually add the prefix, but then it's a bit tedious to add "tw-" to every class in the future. Any suggestions?

bdougie commented 1 year ago

My suggestion see if there is a modern alternative. Perhaps create a vite plugin.

The alternative is creating our own prefixer in the lib folder to make our css collision proof.

Anush008 commented 1 year ago

I tried out building the plugin. Prefixing works as we'd expect it to. https://gist.github.com/Anush008/ffebc6ceb5439dc1ca2996ff7fb88311

The problem being, some of our components inherit styling from GitHub using their classnames. Like SelectMenu js-slash-command-menu in the following example. https://github.com/open-sauced/ai/blob/e95e7a1a0ee3f73f067d69044e97b283608d723c/src/content-scripts/components/AICodeReview/AICodeReviewMenu.ts#L22 https://github.com/open-sauced/ai/blob/e95e7a1a0ee3f73f067d69044e97b283608d723c/src/content-scripts/components/InsightsSelectDropdown/InsightsSelectDropdown.ts#L12 https://github.com/open-sauced/ai/blob/e95e7a1a0ee3f73f067d69044e97b283608d723c/src/content-scripts/components/InsightsSelectDropdown/InsightsSelectDropdown.ts#L14

Prefixing will break this behaviour.