Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

Font family option #25

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

MasGaNo
Copy link
Contributor

@MasGaNo MasGaNo commented Apr 28, 2017

Hi again!

This is a suggestion I made for you.

I'm not a big fan about manage object-fit/position value in different parts: CSS for browsers that implement already these features, and HTML for fallback.

What I suggest is to use the font-family: "object-fit: center; object-position: center;"; rule.
This rule will not have a big impact about font-family of your img (as it cannot have a text inside 😉 ) and it will be ignored by browsers that don't need this polyfill.

So we can manage these properties only in CSS, and for people who use mixin (in SCSS for example), they can easily implement one.

What do you think?

If you think it's a bad idea, just reject this PR, it's not a problem 😃

Again, thank you for this polyfill!

@cee-chen
Copy link
Owner

Heya MasGaNo! First of all, this is really cool, and I'm impressed that you did so much work on this!

I definitely don't think it's a bad idea, but I do think CSS font-family workarounds for polyfills is a personal preference and tends to attract strong opinions 😄 Some devs prefer to dirty their CSS, others prefer to dirty their HTML. For myself, I fall into the "Prefers dirty HTML and clean CSS" camp, hence why my polyfill uses data attributes (also because data attributes are a pretty common paradigm even in regular JS, so it's just what I'm used to!).

Another consideration is that another really awesome polyfill, object-fit-images exists which already uses the CSS font-family paradigm. (Also, I can't believe they listed my polyfill in their comparison chart! Wow! 😱)

That being said! If you'd like, I'd be open to either

  1. Adding another .js file called objectFitPollyfill.fontFamily.js, which users can opt to use instead of the main js functionality

Or,

  1. If you'd like to maintain your own fork, I'd be happy to link to it in the README, so users can choose between the two options, whatever they prefer! :) (Feel free to even publish it to npm or bower as objectFitPolyfill-CSSFont or something, I've got no beef with that! Also, I'm not the greatest at naming things, lol.)

@MasGaNo
Copy link
Contributor Author

MasGaNo commented Apr 29, 2017

Hi constancecchen 😃!

Thank you for your answer ^^

And you are totally right about the fact that this choice should be done by developer and not directly the polyfill 😉

That why I try to keep this script with backward compatibility, but the choice of the font-family has a slight performance issue because it will try to check every image whereas with your data attribute, you can target directly the elements which need this polyfill.

As I'm working more and more with Web Component, and using an architecture by decoupling responsability between JS part, HTML part and CSS part, I prefer using a CSS degradation 😉

I think it's not a bad idea to have different entry point for your script, like the objectFitPolyfill for the initial behaviour with data attribute, the objectFitPolyfill.basic for the basic one, and the objectFitPolyfill.css for the CSS-FontFamily one.

The only problem is to maintain these different scripts if they need some improvement in the future. For the moment it should be cool as your polyfill work perfectly with the different use case I can try!

But if they need to be maintained again, I can suggest you a way to have an objectFitPolyfill.shared for all the global logical code, and to have the different files that will configure the behaviour according to the choice of the developer, during the build phase.

What do you think? 😃

@cee-chen
Copy link
Owner

Hey @MasGaNo! Just wanted to apologize for how long it's taken to get back to you on your PRs - I've been super swamped with side work, but I promise I do fully intend on implementing this comprehensively. (Totally agreed we should not be repeating code across multiple files, so I want to do it the right way round)

In the meanwhile, thanks so much for leaving this PR up as a resource for others!

@joyously
Copy link

joyously commented Feb 2, 2018

I just wanted to jump in to say that there is another way!
I don't like dirty HTML or dirty CSS, so I suggest that this polyfill be adjusted slightly to work with the PrefixFree script. https://github.com/LeaVerou/prefixfree
It would be a simple matter to call your object-fit polyfill only for those CSS selectors that need it, as is done with the conic-gradient polyfill and the Custom Properties polyfill, found at that link.

@cee-chen cee-chen force-pushed the master branch 4 times, most recently from af7f865 to 366a40a Compare April 1, 2019 01:48
Base automatically changed from master to main January 27, 2021 05:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants