-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat(filters): new filters layout #42
Conversation
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
=========================================
- Coverage 96.21% 95.3% -0.92%
=========================================
Files 13 13
Lines 132 149 +17
Branches 11 16 +5
=========================================
+ Hits 127 142 +15
- Misses 5 7 +2
Continue to review full report at Codecov.
|
507b820
to
6d13613
Compare
I played around with this branch some, it seems to work fine. I guess I still have some concerns about this approach purely from a UX perspective... if the filters are part of what makes this application especially cool I find it odd that we're hiding them by default. On mobile this would make sense but for desktop it seems like they weren't taking up that much real estate. Having the content update in the background while the main section of the page is dimmed diminishes the impact too. I don't know, I'm just thinking out loud. |
Yeah I agree there might be some tweaks to it to make it better..one would be to add a proper submit button and hide the navbar at that point, also do that on the form submit event. But not on each input change.. Its quite nice to be able to tweak your filters without constant noise of the items refreshing also so I think there are advantages to it.. I don't love the idea of duplicate stuff for mobile and desktop all the time..so I would try and keep a solution that works cross device. Thats one of Material's mottos also..the one I kind of like the most. Also I would definitely prioritise mobile on it..kind of the approach brought up by google inbox. Its a mobile layout, maybe not optimal for desktop, but mobile comes first. But open to some suggestions and experimentation. |
Dont merge this one yet. I will work on making the sidenav responsive. this way on desktop the filters are always visible as facets. related : angular/components#1130 Basically making it opened by default on desktop (mobile stays as is) and on desktop it woudlnt have this "overlay" effect..would just be on the page..normally sort of like the sidenav here https://material.angular.io/components/sidenav/api |
/** | ||
* Listen to window resize. | ||
*/ | ||
@HostListener('window:resize', ['$event']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure this is supported with universal but I want to make sure..just asked for some info on some of the universal chats to be sure this is not going to break when we start using universal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related angular/angular#15000
although its scroll, if one is supported the other should be too.
I made some changes to the PR. The idea now basically is on larger desktop devices the filters will be visible by default as facets, they can still be hidden if a user wants too (I think its ok to let the user hide them). On mobile it stays as it was as a sidenav and hidden by default. |
Since this is pushing PR's that pass the travis check (we should fix it will open an issue) you can test it on https://contenta-angular.firebaseapp.com |
Fixed the firebase deploy issue finally, but I did look at this on firebase before I fixed it, I like the open by default for desktop behavior a lot more. @joaogarin Is this ready in your view? Should we remove the |
If this previous commit passes its ready. Added responsive testing so we can test mobile behaviour as well in the case something works different for mobile. I will remove the do not merge tag. |
@mrjmd on my side this is ready. I have added some responsive settings for karma that should fix the missing codecov part altough it was not updated here. Right now it tests mobile and desktop versions..its good to have this setup cause this way any responsive behaviour inside the app can be properly tested. |
This pull request introduces a new layout for the filters inside a sidenav.
Marking as do not merge since there is still a minor bug in mobile where the fab is not fixed (not a huge deal..but maybe easy to fix) angular/components#998