-
Notifications
You must be signed in to change notification settings - Fork 771
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
Comments
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. |
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 ;) |
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 would be great to have this. |
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 |
Whoa!! That is a very very compelling reason to ditch this |
If you are getting those impacts by switching moving your flex stuff into your css files then you are using it wrong for sure... 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... |
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.. :) |
@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. |
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 ;) |
Perhaps you could provide a stackblitz showing the problem? |
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. |
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 🙂 |
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. |
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. |
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. |
@fxck you probably have some strange things going on if you have problems with that simple layout.. If you need help perhaps I can take a look!? |
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? |
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.
The text was updated successfully, but these errors were encountered: