search

binaryage / cljs-devtools

A collection of Chrome DevTools enhancements for ClojureScript developers
Other
1.11k stars 51 forks source link

Adjusted styling to work with Firefox #73

Closed SebastianZ closed 1 year ago

SebastianZ commented 1 year ago

With these changes the styles look somewhat the same between the Chrome and the Firefox DevTools.

For Firefox, this change requires the styling changes of https://bugzilla.mozilla.org/show_bug.cgi?id=1818090.

Sebastian

darwin commented 1 year ago

I accepted your PR, but I have reverted some work in subsequent commits.

1) I'm not sure why did you need to change markup layout in markup.cljs? This broke tests. 2) I'm unable to really validate that styling changes do not affect Chrome users. For this I moved Firefox-related styling under new config which gets applied only under Firefox. Also moved some other styles in macros behing -moz- vendor prefix. 3) I accepted removing -webkit- vendor prefix, that should be harmless.

The Firefox styling is still not perfect, feel free to continue on populating firefox-overrides-config in defaults.cljs. I would accept it.

SebastianZ commented 1 year ago

I accepted your PR

Thanks!

1. I'm not sure why did you need to change markup layout in markup.cljs? This broke tests.

Sorry for breaking the tests! I changed it because it was partly broken, caused issues regarding the layout in Firefox and was unnecessarily nested. Actually, I only fixed and simplified one part of the markup, though there are also other places which could be simplified.

2. I'm unable to really validate that styling changes do not affect Chrome users. For this I moved Firefox-related styling under new config which gets applied only under Firefox.

Ok, though note that with those changes I also tried to improve the layout in Chrome.

Also moved some other styles in macros behing -moz- vendor prefix.

That won't work. Firefox doesn't support those prefixed properties.

The Firefox styling is still not perfect, feel free to continue on populating firefox-overrides-config in defaults.cljs. I would accept it.

Yes, that's due to the changes required on Firefox side and the prefixes you added. The Firefox changes already got merged a few days ago, so most of the layout already works with that. I'll see how I can fix the things related to the prefixed properties and come up with another PR.

Thanks again for taking the time for the review and doing those changes!

Sebastian