Skip to content

Commit

Permalink
fix(fxLayoutGap): correctly handle lack of fallback value (#1037)
Browse files Browse the repository at this point in the history
In most directives, style generation and removal happens in
a contained cycle: styles are generated, cached, and then
removed when no longer needed. This is facilitated by caching
the results of the `StyleBuilder`s for each directive in a
local object on the directive, and then removing those styles
when called.

In the case of `fxLayoutGap`, most of the style generation is
applied to the children, and so in most cases just removing the
parent styles is insufficient, and in other cases, just
ineffective (because no parent styles are actually generated).
To fix this, we override the default style clearing method,
and replace it with one that actually removes the child styles
(and the parent styles if any are present).

Fixes #1011
  • Loading branch information
CaerusKaru authored Mar 17, 2019
1 parent c23621c commit ce9b989
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 16 deletions.
36 changes: 36 additions & 0 deletions src/lib/flex/layout-gap/layout-gap.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ describe('layout-gap directive', () => {

let nodes = queryFor(fixture, '[fxFlex]');
expect(nodes.length).toEqual(3);
mediaController.activate('sm');
expectEl(nodes[0]).not.toHaveStyle({'margin-right': '*'}, styler);
expectEl(nodes[1]).not.toHaveStyle({'margin-right': '*'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'margin-right': '*'}, styler);
Expand All @@ -384,6 +385,11 @@ describe('layout-gap directive', () => {
expectEl(nodes[1]).toHaveStyle({'margin-right': '24px'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'margin-right': '24px'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'margin-right': '0px'}, styler);

mediaController.activate('sm');
expectEl(nodes[0]).not.toHaveStyle({'margin-right': '*'}, styler);
expectEl(nodes[1]).not.toHaveStyle({'margin-right': '*'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'margin-right': '*'}, styler);
});

it('should set gap with responsive layout change', () => {
Expand Down Expand Up @@ -503,6 +509,36 @@ describe('layout-gap directive', () => {
expectNativeEl(fixture).toHaveStyle(expectedMargin, styler);
});

it('should set gap without fallback', () => {
let template = `
<div fxLayoutAlign='center center' fxLayoutGap.md="24px grid">
<div fxFlex></div>
<div fxFlex></div>
<div fxFlex></div>
</div>
`;
createTestComponent(template);
fixture.detectChanges();

let nodes = queryFor(fixture, '[fxFlex]');
expect(nodes.length).toEqual(3);
mediaController.activate('sm');
expectEl(nodes[0]).not.toHaveStyle({'padding': '*'}, styler);
expectEl(nodes[1]).not.toHaveStyle({'padding': '*'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'padding': '*'}, styler);

mediaController.activate('md');
fixture.detectChanges();
expectEl(nodes[0]).toHaveStyle({'padding': '0px 24px 24px 0px'}, styler);
expectEl(nodes[1]).toHaveStyle({'padding': '0px 24px 24px 0px'}, styler);
expectEl(nodes[2]).toHaveStyle({'padding': '0px 24px 24px 0px'}, styler);

mediaController.activate('sm');
expectEl(nodes[0]).not.toHaveStyle({'padding': '*'}, styler);
expectEl(nodes[1]).not.toHaveStyle({'padding': '*'}, styler);
expectEl(nodes[2]).not.toHaveStyle({'padding': '*'}, styler);
});

it('should add gap styles correctly for rtl', () => {
fakeDocument.body.dir = 'rtl';
let template = `
Expand Down
44 changes: 28 additions & 16 deletions src/lib/flex/layout-gap/layout-gap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,21 @@ export class LayoutGapDirective extends BaseDirective2 implements AfterContentIn
}
}

/** We need to override clearStyles because in most cases mru isn't populated */
protected clearStyles() {
const gridMode = Object.keys(this.mru).length > 0;
const childrenStyle = gridMode ? 'padding' :
getMarginType(this.directionality.value, this.layout);

// If there are styles on the parent remove them
if (gridMode) {
super.clearStyles();
}

// Then remove the children styles too
this.styleUtils.applyStyleToElements({[childrenStyle]: ''}, this.childrenNodes);
}

/** Determine if an element will show or hide based on current activation */
protected willDisplay(source: HTMLElement): boolean {
const value = this.marshal.getValue(source, 'show-hide');
Expand Down Expand Up @@ -262,28 +277,25 @@ function buildGridMargin(value: string, directionality: string): StyleDefinition
return {'margin': `${marginTop} ${marginRight} ${marginBottom} ${marginLeft}`};
}

function buildGapCSS(gapValue: string,
parent: {directionality: string, layout: string}): StyleDefinition {
let key, margins: {[key: string]: string | null} = {...CLEAR_MARGIN_CSS};

switch (parent.layout) {
function getMarginType(directionality: string, layout: string) {
switch (layout) {
case 'column':
key = 'margin-bottom';
break;
return 'margin-bottom';
case 'column-reverse':
key = 'margin-top';
break;
return 'margin-top';
case 'row':
key = parent.directionality === 'rtl' ? 'margin-left' : 'margin-right';
break;
return directionality === 'rtl' ? 'margin-left' : 'margin-right';
case 'row-reverse':
key = parent.directionality === 'rtl' ? 'margin-right' : 'margin-left';
break;
return directionality === 'rtl' ? 'margin-right' : 'margin-left';
default :
key = parent.directionality === 'rtl' ? 'margin-left' : 'margin-right';
break;
return directionality === 'rtl' ? 'margin-left' : 'margin-right';
}
margins[key] = gapValue;
}

function buildGapCSS(gapValue: string,
parent: {directionality: string, layout: string}): StyleDefinition {
const key = getMarginType(parent.directionality, parent.layout);
const margins: {[key: string]: string | null} = {...CLEAR_MARGIN_CSS};
margins[key] = gapValue;
return margins;
}

0 comments on commit ce9b989

Please sign in to comment.