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 icons displayed in BCD cells #6811

Merged
merged 2 commits into from
Aug 11, 2022
Merged

Fix icons displayed in BCD cells #6811

merged 2 commits into from
Aug 11, 2022

Conversation

queengooborg
Copy link
Collaborator

This PR fixes the rendering of the prefix, altname and flags icons on BCD cells to ensure they aren't unintentionally shown when not applicable. This should help reduce issues and confusion such as mdn/browser-compat-data#17208.

@danielhjacobs
Copy link
Contributor

danielhjacobs commented Aug 3, 2022

The reason for the switch in logic was pages like https://developer.mozilla.org/en-US/docs/Web/CSS/flex-flow#browser_compatibility. This will go back to showing Firefox as needing a vendor prefix, since support for a vendor prefix was added after support without a vendor prefix. That's why my proposed fix was #6663

See also #5721

@queengooborg
Copy link
Collaborator Author

This will go back to showing Firefox as needing a vendor prefix, since support for a vendor prefix was added after support without a vendor prefix.

Actually, this PR's intent is to do the opposite, and NOT show needing a vendor prefix when it shouldn't. On https://developer.mozilla.org/en-US/docs/Web/CSS/transition#browser_compatibility, such an issue can be found. It seems that #6663 fixes a good chunk of the problems, but there's still one or two that can be found.

One of the benefits of this approach is that it will always match the result from the getCurrentSupport() function, the same function used to select what version to display in the cell. I haven't seen any issues so far with the function, so I've trusted it to make the right call on pages.


To help show the changes this PR makes, this is the transition CSS property page, before and after:

Screen Shot 2022-08-03 at 07 15 12

Screen Shot 2022-08-03 at 07 15 39

And to show that other pages like flex-flow are unaffected:

Screen Shot 2022-08-03 at 07 16 02

@danielhjacobs
Copy link
Contributor

Oh right, I forgot I added a ranking system in #5946 to that function. This makes sense then, yeah, Using it wasn't an option before since I hadn't changed it yet.

In that case, my feedback is this: Would this work the same if you remove those three functions and switch back to the pre- #5721 statements?

@danielhjacobs
Copy link
Contributor

In other words, just supportItem.prefix, supportItem.alternative_name, and supportItem.flags

@queengooborg
Copy link
Collaborator Author

Sure thing, changes coming right up!

@danielhjacobs
Copy link
Contributor

Note: Also fixes mdn/browser-compat-data#16839 and fixes mdn/browser-compat-data#16833

Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@fiji-flo fiji-flo merged commit 0d96355 into mdn:main Aug 11, 2022
@queengooborg queengooborg deleted the bcd-icons branch August 11, 2022 17:58
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.

3 participants