Skip to content

Adjusted styling to work with Firefox#73

Merged
darwin merged 1 commit into
binaryage:masterfrom
SebastianZ:fixed-styling-for-firefox
Feb 26, 2023
Merged

Adjusted styling to work with Firefox#73
darwin merged 1 commit into
binaryage:masterfrom
SebastianZ:fixed-styling-for-firefox

Conversation

@SebastianZ

Copy link
Copy Markdown
Collaborator

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
darwin merged commit efe3401 into binaryage:master Feb 26, 2023
@darwin

darwin commented Feb 26, 2023

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator Author

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

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