Container query addition #31
Conversation
Blackbaud-TrevorBurch
requested review from
Blackbaud-AlexKingman and
Blackbaud-SteveBrush
as code owners
|
FYI - when the flyout goes into "full screen mode" the mixins need to be updated as well. |
…to media-container-addition and updated dependencies
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
==========================================
+ Coverage 99.71% 99.76% +0.05%
==========================================
Files 10 11 +1
Lines 348 429 +81
Branches 49 67 +18
==========================================
+ Hits 347 428 +81
Misses 1 1
Continue to review full report at Codecov.
|
src/app/public/modules/flyout/flyout-media-query.service.spec.ts
Outdated
Show resolved
Hide resolved
src/app/public/modules/flyout/flyout-media-query.service.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Some initial thoughts. Great work on this!
I think this feature merits a visual test; ideally, some colorful columns (like you did for @skyux/theme) to demonstrate that a child component within the flyout is honoring the "new" SkyMediaBreakpoints values, instead of the actual screen size.
| this.renderer.removeClass(flexEl, 'sky-responsive-container-md'); | ||
| this.renderer.removeClass(flexEl, 'sky-responsive-container-lg'); | ||
|
|
||
| if (breakpoint === SkyMediaBreakpoints.xs) { |
There was a problem hiding this comment.
Mind making this a switch statement for readability?
There was a problem hiding this comment.
Another improvement: set the desired CSS class name to a variable, and only call this.renderer.addClass() once, at the end of the switch statement.
| @@ -164,6 +177,16 @@ export class SkyFlyoutComponent implements OnDestroy, OnInit { | |||
| event.stopPropagation(); | |||
| } | |||
|
|
|||
| @HostListener('window:resize', ['$event']) | |||
| public onWindowResize(event: any): void { | |||
| const xsBreakpointMaxPixels = 767; | |||
There was a problem hiding this comment.
Pulling the 767 value out of the SkyFlyoutMediaQueryService seems redundant (and potentially problematic since we'd be managing the same value in two places). Any way to keep this value buried in the service?
There was a problem hiding this comment.
This is an interesting one. I guess we would a method to the media query service that takes in both the window size and the flyout width and then that can do this logic inside of that. Just seemed odd to me to do that but I also understand the magic number concern.
There was a problem hiding this comment.
You could create a new method that checks that a given width falls within a specific breakpoint and keep the same if/else structure you have here.
(This new method could even potentially be used by the setBreakpoint method.)
There was a problem hiding this comment.
Good call. I've done that but just made four methods - one for each breakpoint.
I can combine them all if you like them combined during review.
No description provided.