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

Consider using classes in favor of inline styles where possible #1299

Open
Enngage opened this issue Oct 20, 2020 · 18 comments
Open

Consider using classes in favor of inline styles where possible #1299

Enngage opened this issue Oct 20, 2020 · 18 comments
Labels
discussion Further discussion with the team is needed before proceeding rfc

Comments

@Enngage
Copy link

Enngage commented Oct 20, 2020

Hey all,

I was wondering if there are any plans or considerations of using classes instead of inline styles to improve rendering performance? As is mentioned in best practices for performance (https://github.com/angular/flex-layout/wiki/Performance-Considerations), the use of inline styles is heavy on computation and can cause signififant slowdowns even on < 100 table rows when used on mobiles.

Having e.g. 100 css classes (1 for each percentage point) wouldn't be a problem for majority of people, and the performance gains of this alone would be very much worth the effort and slightly increased size of library.

There is probably not much you can do for pixel based flex, but thats ok.

@CaerusKaru
Copy link
Member

Inline styles were chosen to, above all, provide a way to give the highest specificity to the styles generated. That way, for those not familiar with CSS, applying flex styles would, 99% of the time, give you the intended styling.

I would give thought into a class-based approach for the redesign in #1185, but not in the present design. It poses too much of either a breaking change, or a bloat to the library where size is at a premium.

@CaerusKaru CaerusKaru added discussion Further discussion with the team is needed before proceeding rfc labels Oct 22, 2020
@Enngage
Copy link
Author

Enngage commented Jan 10, 2021

I'm really hoping that people using Angular know what CSS is and how it works : ) I don't think Angular would be a good fit for people without CSS knowledge so catering to these users makes little sense to me (well, unless you have some reports / data proving me wrong).

What I'm finding out more and more often is that devs / users are complaining that angular is slow and quite often the reason is the incorrect use of this library. I'm an Angular fan and find it a bit disappointing because Angular, when used properly, is pretty fast.

It's very easy to use this library wrong and I had / have to rewrite a lot of code just to achieve acceptable performance levels.

I guess what I'm saying is that it would be awesome to see some performance improvements to this library ;)

@stagefright5
Copy link

Totally agree with @Enngage . In my team, we are actually thinking about completely moving away from this library for the redering performance reasons and write our own custom flex utility classes.
It has become unbearably slow, especially when resizing.
I am not saying that our devs have used the library optimally, but the problem is that the library API looks so innocent that you would not even think about it.

It would be great to have this.

@Enngage
Copy link
Author

Enngage commented Feb 19, 2021

I've actually moved to a completely custom implementation of flex based on CSS classes since my last message, and the performance gain is very noticable and even the google page speed score went up 25-35 just because of this :)

@stagefright5
Copy link

I've actually moved to a completely custom implementation of flex based on CSS classes since my last message, and the performance gain is very noticable and even the google page speed score went up 25-35 just because of this :)

Whoa!!

That is a very very compelling reason to ditch this

@mackelito
Copy link

If you are getting those impacts by switching moving your flex stuff into your css files then you are using it wrong for sure...
By getting those numbers I assume you have a really large DOM... and in that case the recommendation by this lib is to move the bigger repeating parts to your css files...

https://github.com/angular/flex-layout/wiki/Performance-Considerations

I would argue that if you have a large DOM then you are doing it wrong.. use virtual scrolling or similar techniques for better performance...

@mackelito
Copy link

I'm not saying that this is a bad idea! (I would like to see this my self) but to blame you performance issues on this alone is not seeing the whole picture.. :)

@Enngage
Copy link
Author

Enngage commented Feb 19, 2021

@mackelito, not sure if you have used this in some non-trivial project, but the performance starts to go down very quickly. It really depends on how many rules you set up, but when you optimize for extra large, large, tablet like & mobile screens and use different sizes, aligns etc. you will soon see the difference. And I'm not talking about a huge DOM, but relatively modest layout when you use fxFlex for overall layout, menu items and actual content. Virtual scrolling is good if you have thousands of rows, but not when you have 50 rows and it starts lagging on mobiles phones.

I've linked the Performance considerations link in my very first post, and it was what I started using, but I soon found that that if I have to optimize everything with this approach, I might as well just create my own library for flex and use that.

It's a shame though because when I started using this library I really liked it. Sadly I didn't realize all the performance implications of using it.

@mackelito
Copy link

I have used this lib in large projects, many users and high SEO focus... Teams with 5-15 developers.. so yes I have used it in non-trivial projects ;)

If you have lagging issues on 50 rows when using this lib I would start by looking on the rendering and what actually causes the performance problem :)

I would almost bet you money that you can get really nice performance using this lib ;)

@mackelito
Copy link

Perhaps you could provide a stackblitz showing the problem?

@stagefright5
Copy link

I would almost bet you money that you can get really nice performance using this lib ;)

The performance should be a given rather than something that users of the lib has to optimize for and "can" be achieved.

That said, may be it is the case that we have actually written extremely sh**ty code. I will try debugging the problem, try to find the actual root cause and update my findings here.

@mackelito
Copy link

Agreed that it would be nice to have no performance impact when using this library..

But for us the trade offs between perform and developer experience is a no brainier 🙂

@Enngage
Copy link
Author

Enngage commented Feb 20, 2021

It's not just those 50 rows, it's more of a combination of entire layout, several layers of menus and finally columns within layout and actual data. One of the things that had significant impact on the performance were those conditional styles & alignments. Removing e.g. centering of certain elements had huge impact on the performance. If you are happy with the performance, all the better ;) I'm just saying it really depends on the complexity of your code.

I have to admit that before starting using fxFlex I had little experience with flex on its own. We've always used some 3rd party library, but once I got a bit more familiar with it and built our own flex library based on CSS classes (unlike inline styles that this library uses) we are having a admittedly better developer experience and on top of that significantly better performance. I don't think I'll ever come back to this library, but it's definitely a shame.

I think this library is really good for simple projects or for people not familiar with flex, but as soon as your needs grow or you yourself become familiar with flex, you will end up in the same situation.

@samal-rasmussen
Copy link

samal-rasmussen commented Jun 10, 2021

I've just removed fxLayout from a complex settings view and it halved the render time.

It was a table with 29 rows and about 20 columns. Inside each cell there was a input and a checkout that were laid out using fxLayout. Looking in the browser dev tools profiler I could see that applying the inline styles was causing layout thrashing for each cell. Changing to using css based flex fixed the layout thrashing and halved the time spent rendering.

@fxck
Copy link

fxck commented Jun 10, 2021

Please do yourself a favor and get rid of flex-layout as soon as possible, it HUGELY affects performance, especially when nested inside other components. Imagine 10 cards in a grid layout, with 3 components inside, each having few components with another grid inside. It's not unreasonable amount of components and grid layouts required along with it, even with just a little more complex UI elements. If you rely on flex-layout for this, you are going to make your app unusable.

We realized this way too late and the refactoring was costly. Both in time and mental health. Get out while you can. CSS Grids are a little less flexible, but much more performant alternative.

image

@mackelito
Copy link

@fxck you probably have some strange things going on if you have problems with that simple layout..
I have been using this lib for way more complexed layouts without performance issues.. (a score of 90+ in lighthouse.. and the layout was not the issue)..

If you need help perhaps I can take a look!?

@fxck
Copy link

fxck commented Jun 11, 2021

angular/angular#38184

@mackelito
Copy link

It would be really helpful if you had a stackblitz or something where we could see the problem being reproduced as @pkozlowski-opensource could not reproduce the issue then either some bits and peaces are missing in the reproduction or there is something in your setup that you perhaps forgot to tell about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Further discussion with the team is needed before proceeding rfc
Projects
None yet
Development

No branches or pull requests

6 participants