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

feature(dependencies): add @angular/flex-layout (closes #239) #334

Merged
merged 9 commits into from
Feb 15, 2017

Conversation

emoralesb05
Copy link
Contributor

@emoralesb05 emoralesb05 commented Feb 10, 2017

Description

Including @angular/flex-layout as part of CovalentCoreModule and started initial migration to leverage it. #239

This is meant to show both flex libs can co-exist in covalent as we migrate into @angular/flex-layout.

For the time being, we will be fixed to a specific version since its a beta release. (beta.5)

What's included?

  • chore(): add @angular/flex-layout as as a @covalent/core dependency
  • feature(paging): leverage flex-layout in paging module.
  • feature(search): leverage flex-layout in search module.

Test Steps

General Tests for Every PR

  • ng serve --aot still works.
  • npm run lint passes.
  • npm test passes and code coverage is not lower.
  • npm run build still works.

@emoralesb05 emoralesb05 added this to the Beta 2 milestone Feb 10, 2017
@emoralesb05 emoralesb05 changed the title feature(dependencies): add @angular/flex-layout feature(dependencies): add @angular/flex-layout (closes #239) Feb 10, 2017
@kyleledbetter
Copy link
Contributor

Code & UX LGTM 👍

I've got questions more around the performance & cascading implications of flex-layout using inline styles instead of CSS stylesheets, that are mainly just unknowns for me, since we use flex layout not only for layouts but for every single UI element. That's a ton of extra inline HTML styles...

Obviously I can see all the benefits of this being in JavaScript with all the power of Angular2 with media queries, change detection, etc, which is why I'm excited about it

I'll do more research from their wiki and more testing on my own.

@joshwiens
Copy link
Contributor

joshwiens commented Feb 11, 2017

@kyleledbetter - I started using fxFlex at work back when it was still private and you are correct there is definitely a balance to be found between inline & CSS.

What I ended up settling on was using fxFlex for container components layout & anything where the benefits ObservableMedia are too good to pass up but I was always careful with that. Creating a subscription on <td-expansion-panel> for instance would undoubtedly lead to issues logged in Github for someone that has 50 of them rendering in a view and can't figure out where the Jank came from. It's definitely an all things in moderation kind of tool.

All the low level styles for components I ended up moving back to .css, not so much because of a performance issue but simply because the bloat in my HTML styles was messing with my OCD.

@emoralesb05 - I played around with fxFlex in Covalent before I opened #239 so I have most of it converted all be it out of date. The approach I took personally was to just replace existing tags with their fxFlex counterpart without changing or adding additional styling / ObservableMedia / responsive ngClass.

From Kyle's comment it sounds like that approach would be optimal to kind of ease into the pool before heading out to the media query deep end. I can PR that in once the basic support lands assuming the above is accurate.

@emoralesb05
Copy link
Contributor Author

@d3viant0ne Yeah, it feels like we might have a need for both things to coexist for now.

I still think its greatly needed (it has awesome features) but we might stick to ours for internal usage in our components and use @angular/flex-box for more granular stuff maybe within applications and docs.

@kyleledbetter
Copy link
Contributor

@emoralesb05 i'm cool with merging in for use if we revert search & paging to use the css classes, that way we can do more testing downstream in products

@emoralesb05
Copy link
Contributor Author

@kyleledbetter yep, will revert those so we can just merge it

@kyleledbetter kyleledbetter merged commit 171a69d into develop Feb 15, 2017
@kyleledbetter kyleledbetter deleted the feature/flex-layout branch February 15, 2017 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants