Flex-layout: Falsy checking "display" in _getDisplayStyle

Created on 30 May 2017  路  7Comments  路  Source: angular/flex-layout

Totally understand this being a low priority thing given I am off in i'm off in uncharted waters testing Angular with Jest. While i'm obviously not going to be testing my layout using a jsdom driven test runner, there really isn't a reason why fxLayout & Jest can't play nice together.

As that relates to flex-layout, there are two small issues that have arisen after changing the test runner.

The first issue is related to window.matchMedia and not a problem within flex-layout which was solved with a global jest mock ( may be of value to someone in the future testing w/ Jest ).

Object.defineProperty(window, 'matchMedia', { value: jest.fn(() => { return { matches: true }; }) });

The second issue could be handled within flex-layout.

_Problem_

  protected _getDisplayStyle(source?: HTMLElement): string {
    let element: HTMLElement = source || this._elementRef.nativeElement;
    let value = (element.style as any)['display'] || getComputedStyle(element)['display'];
    return value.trim();
  }

Given the above in flexbox/api/base.ts an element that doesn't have a display style is going to be undefined. When you attempt to .trim() undefined you will end up with ...

TypeError: Cannot read property 'trim' of undefined

_Proposed Solution_

Default the return value for anything falsy. iirc the default value for the css display property is inline so perhaps default the return value to that?

  protected _getDisplayStyle(source?: HTMLElement): string {
    let element: HTMLElement = source || this._elementRef.nativeElement;
    let value = (element.style as any)['display'] || getComputedStyle(element)['display'];
    return value ? value.trim() : 'inline';
  }

If you agree conceptually, i'll get you a small repro. if you want and a PR to resolve the issue.

has pr

Most helpful comment

@kara any plan to release a new version of flex-layout soon? This bug is blocking for us :)

All 7 comments

@d3viant0ne - I like these thoughts. Plz push a PR.

FYI - I thought the default display values was block ?

I could totally be wrong, i'll double check the default before I PR the change.

Yup, I had my element types / defaults backwards. Default for anything layout related is block

@kara any plan to release a new version of flex-layout soon? This bug is blocking for us :)

@victornoel - There is quite a bit left to do in the beta.9 milestone. I would guess we aren't going to see another release until some of the big ticket items are resolved.

@d3viant0ne ok, thanks :)

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_This action has been performed automatically by a bot._

Was this page helpful?
0 / 5 - 0 ratings

Related issues

jcoronel94 picture jcoronel94  路  4Comments

spottedmahn picture spottedmahn  路  5Comments

mackelito picture mackelito  路  4Comments

manuelbachl picture manuelbachl  路  5Comments

JadedEric picture JadedEric  路  4Comments