search

liferay / clay

A web implementation of the Lexicon Experience Language
http://clayui.com
Other
208 stars 483 forks source link

Remove Support for IE11 in @clayui/css 3.x #3741

Closed jbalsas closed 4 years ago

jbalsas commented 4 years ago

As part of Remove IE11 Support, we should be taking steps to remove IE11 support, hacks and workarounds from our code.

I personally think the benefit of removing code here is not enough to warrant us forking the library, so my first inclination is to stay put. In any case, I thought we should have this conversation :)

One option that came to mind was to add a Sass variable $ie_support and wrap all those IE specific fixes in an if clause. For DXP 7.4.x we can set it to false to save some extra bytes...

@wincent, @pat270, @bryceosterhaus, @matuzalemsteles, anyone else... thoughts on this?

wincent commented 4 years ago

beautiful

A beautiful summary of the trade-offs.

kresimir-coko commented 4 years ago

matuzalemsteles commented 4 years ago

Good summary I also don't see many specific things in the components that are for IE 11, I agree not to do anything about it for the components, if you look at the browser: ie label, there were few bugs reported but the vast majority of the site.

I don't have that much vision of hacks for CSS but @pat270 could say better how much we have of it and what we can benefit from if we were to work on it in a major version maybe for CSS. I like the idea of ​​the variable but a question since we really intend to eliminate support maybe we could go with a 4.x version of CSS and remove these hacks without the flag?

pat270 commented 4 years ago

For Clay CSS, I think we should up the major version. Having the setting $ie_support implies any new component or update we do needs to work in IE11 if that setting is set to true.

bryceosterhaus commented 4 years ago

Like @matuzalemsteles said, I don't think the react components have much specific for ie11, but I believe clay-css has quite a bit. I think I would lean towards agreeing with @pat270, that its safer to just up a major version for clay-css and not worry about setting the ie_suppot flags

jbalsas commented 4 years ago

So, upping clay-css would mean:

  1. We would be using @3.x for components and @4.x for clay-css which is somewhat confusing.
  2. We'll have to maintain 2 versions of @clay/css simultaneously. It'll be hard to keep track of fixes going to one branch or the other.
  3. If we wanted to avoid 1, we'd need to up versions for all clay packages, which would cause 2 but for every package.

Based on past experience, I think the version divergence is something to avoid until really necessary. I honestly think the costs of moving to 4.x is greater than the benefit of removing those bits of IE code.

bryceosterhaus commented 4 years ago

Ah I see, I was thinking we could get away with just not worrying about IE, but seems like we have to keep it in mind for older dxp versions(unless we can find a way around this?). In that case, I agree with you and that it's probably easier to maintain a single version rather than support two different versions.

pat270 commented 4 years ago

Removing support for IE11 is a major change, but if it's too much hassle I rather do the $ie_support thing instead of a complete removal. A complete removal will make it impossible to apply patches for 7.3. Clay CSS versions for 7.3 will stop at whatever version the removal lands on (e.g., 3.18.1). I suppose one way we can circumvent is IE updates and general bug fixes can go in the theme 🤷‍♂️ ?

jbalsas commented 4 years ago

I suppose one way we can circumvent is IE updates and general bug fixes can go in the theme 🤷‍♂️

That's actually a good idea... if we're okay with dropping IE11 support in Clay mid 3x, then we could move every bit of IE-specific CSS to DXP 7.3.x and cut a 3.19.0 version that does not support it. I think this is acceptable seeing as DXP is our main (and likely only) customer.

One alternative way in case someone is really against this would be to move all IE CSS to a different sass entry (ie.scss). That means that for customers, upgrading to 3.19.0 would mean also including the ie.css file if they want IE compatibility with clay. In DXP, this would also be done at the theme level.

wincent commented 4 years ago

and cut a 3.19.0 version that does not support it.

Got to be careful with semver though... if we don't follow it, we risk eroding people's trust. In an idea world, everybody knows that you can trust a Liferay npm package not to have any breaking changes in non-major updates (ie. ^ ranges are safe).

jbalsas commented 4 years ago

[...] we risk eroding people's trust

Yeah, that was my point about who's consuming clay... could you make your preference on this matter more explicit? Based on all the options, what would be your preferred path forward?

(We do have 150 ⭐ now that I realize 😂 )

But based on npm:

Screen Shot 2020-10-08 at 09 08 32

And those 2 are both us 🤷

No usage of components outside of the @clayui org either. See @clayui/icon

Screen Shot 2020-10-08 at 09 09 42
kresimir-coko commented 4 years ago

We also have 420 weekly downloads of @clayui/css, that requires an even better celebration 🌱

jbalsas commented 4 years ago

We also have 420 weekly downloads of @clayui/css, that requires an even better celebration 🌱

That's probably how many distinct ant alls we run 😂

wincent commented 4 years ago

We also have 420 weekly downloads of @clayui/css, that requires an even better celebration 🌱

These are almost certainly > 95% downloads by Liferay employees building liferay-portal. You can see the same in eslint-config-liferay, another one that clearly is only used by us, which "boasts" 1,234 weekly downloads.

Screenshot 2020-10-08 -093820-bd9ghzWp@2x

wincent commented 4 years ago

Yeah, that was my point about who's consuming clay... could you make your preference on this matter more explicit? Based on all the options, what would be your preferred path forward?

My preference — based on thinking about this for 5.1 seconds — would be:

I think this is the "have our cake and eat it too" path, because we implement a breaking change but we make it super easy for people to workaround it (by adding a file).

It is true that for as long as this ie.scss file exists, we have to bear the cost of maintaining it, but the reality is that the cost should be low and diminish over time because:

But, that's just like, my opinion, man.

opinion

As @pat270 is the one who is most likely to bear the brunt of the costs of this decision, and he's the one with the most insight into the difficultly of doing such a refactoring, I think we should defer to his judgment.

jbalsas commented 4 years ago

Be honest and respect semver; ie. cut @clayui/css v4.

Based on that, how would you propose we maintain old DXP branches?

In fact, I guess that if we break @clayui/css that could mean that all packages that depend on it can also include the breaking change and thus need to be updated to v4?

wincent commented 4 years ago

My thinking, again based on 2.6 seconds of careful reflection, is that the introduction of a breaking change in @clayui/css and what happens in DXP are actually two separate things. That is, we can do what you said in your second bullet point:

  • Update older DXP releases to v4 + ie.css?

ie. it is a breaking change, but we shield users of DXP from it. The only people who have to worry about ie.scss are the rare breed of users that consume Clay independently.

  • Do we need a branch for 3.x to maintain it over time and keep fixes in sync when necessary?

As a matter of practice, we could fork a branch just like we did for 2.x, but that case was a bit different because we moved all of Clay to v3.x. In this instance, we're only talking about bumping @clayui/css, and it stays right there on master along with everything else. (Because I don't see why we'd need to go back and apply maintenance fixes to @clayui/css v3.x — if somebody wants a fix, they upgrade to v4.x, and if they need to isolate themselves from the breaking change, the use the ie.scss file.)

  • Are we fine with @clayui/css@4.x and @clayui/icon@3.x? It feels like this can become a bit of a maintenance nightmare if we can't make coherent sense of package versions about what works with what...

I think we're already in that situation, ever since we decided not to do atomic releases. Just look at all the different version numbers in frontend-taglib-clay. That is, this doesn't seem like it would make things much worse?

Just to finish with what I said before...

@pat270 is the one who is most likely to bear the brunt of the costs of this decision, and he's the one with the most insight into the difficultly of doing such a refactoring, I think we should defer to his judgment.

pat270 commented 4 years ago

In fact, I guess that if we break @clayui/css that could mean that all packages that depend on it can also include the breaking change and thus need to be updated to v4?

^^^^^^ This!!

We will have design changes in 7.4 (always happens unless you have secret info). This will require changes in markup, css, and components. How do we shield 7.3 users from inadvertent breaking changes in 7.4?

Our React components don't have much IE stuff, but patching to 7.3 still applies. If we remove IE related code from our React components, how do we patch without new features possibly causing unintended consequences?

I think forking a 3.x branch of everything is the safest. If we still don't want to do that I prefer:

1) Leave IE stuff as is in Clay CSS add new features that still work in IE. Remove IE in Clay 4. 2) $ie_support or _ie.scss, remove IE in Clay 4.

Removing IE isn't going to reduce the file size by much, IE11 has good flexbox support.

jbalsas commented 4 years ago

^^^^^^ This!!

With "can also include breaking changes" I didn't mean it in the sense that we had the option to do so 😂

We will have design changes in 7.4 (always happens unless you have secret info).

I don't have secret info, but I honestly hope there won't be. I think our visual identity is by now mature-ish stable and we shouldn't expect it to change, at least until a major release. @victorvalle might have more insights about that, though.

Removing IE isn't going to reduce the file size by much, IE11 has good flexbox support.

Based on this, I'd say the best thing to do is to do nothing.

victorvalle commented 4 years ago

No changes that I know or can predict right now. We would let you know with time if that happens.

jbalsas commented 4 years ago

Okay, closing as discarded :)