fix(fxFlexOffset): use parent flow direction for margin property #369
Conversation
ThomasBurleson
force-pushed
the
thomas/issue-328
branch
4 times, most recently
from
6c8b0f3
to
00cd78a
Compare
|
@crisbeto - would you mind reviewing these? |
| let isBox = _.hasStyle(dom, 'margin-left', '32px'); | ||
| let hasFlex = _.hasStyle(dom, 'flex', '1 1 1e-09px') || // IE | ||
| _.hasStyle(dom, 'flex', '1 1 1e-9px') || // Chrome | ||
| _.hasStyle(dom, 'flex', '1 1 0.000000001px') || // Safari |
There was a problem hiding this comment.
Perhaps this should be rounded down in the implementation for consistency?
There was a problem hiding this comment.
required. will not change.
| _.hasStyle(dom, 'flex', '1 1 0px'); | ||
|
|
||
| expect(isBox).toBeTruthy(); | ||
| expect(hasFlex).toBeTruthy(); |
There was a problem hiding this comment.
Since hasStyle returns a boolean, you can expect(hasFlex).toBe(true).
| }); | ||
|
|
||
|
|
||
| describe('', () => { |
There was a problem hiding this comment.
This one's missing a title. If the test inside is standalone, they can be moved outside.
| * Cleanup | ||
| */ | ||
| ngOnDestroy() { | ||
| super.ngOnDestroy(); |
There was a problem hiding this comment.
AFAIK this isn't necessary as of Angular 2.4.
There was a problem hiding this comment.
will leave as explicit indicator to call overriden, super class method.
| @@ -84,7 +103,43 @@ export class FlexOffsetDirective extends BaseFxDirective implements OnInit, OnCh | |||
| // Protected methods | |||
| // ********************************************* | |||
|
|
|||
| /** The flex-direction of this element's host container. Defaults to 'row'. */ | |||
| protected _layout = 'row'; | |||
There was a problem hiding this comment.
This one should be strongly typed.
There was a problem hiding this comment.
Why? I thought explicit value assignment defines type and negates need to declare it.
There was a problem hiding this comment.
I think that this one will be inferred to string which could be anything. Aren't the only possible values row and column?
There was a problem hiding this comment.
Now I understand. We do have value validators. I think you found a potential bug since we are not using that here NOR checking for row-reverse or column-reverse.
I will fix.
There was a problem hiding this comment.
Fixed with isFlowHorizontal() call on line 162.
ThomasBurleson
force-pushed
the
thomas/issue-328
branch
from
00cd78a
to
1d4191f
Compare
ThomasBurleson
added
pr: lgtm
`fxFlexOffset` assigns inline `margin-left` style; assuming default flow directions == 'row'. If the parent flow direction == 'column', then a `margin-top` should be used. Also added a subscription to an optional parent element LayoutDirective; which will trigger the FlexOffsetDirective to update the inline styles to match the current flow direction. > Note: if `fxFlexOffset` is used **without** a parent flexbox styling (set via css or directive), then a `display:flex; flex-direction:row;` will be auto assigned to the fxFlexOffset host element's parent. Fixes #328.
ThomasBurleson
force-pushed
the
thomas/issue-328
branch
from
1d4191f
to
4dcae0d
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fxFlexOffsetassigns inlinemargin-leftstyle; assuming default flow directions == 'row'. If the parent flow direction == 'column', then amargin-topshould be used.Also added a subscription to an optional parent element LayoutDirective; which will trigger the FlexOffsetDirective to update the inline styles to match the current flow direction.
Fixes #328.