fix(refresher): border should only show when pulled

[Manduro]

Short description of what this resolves:

See #10994

Changes proposed in this pull request:

• When Content has a Refresher, the calculated top margin is reduced by 1px to hide the border.

Ionic Version: 3.x

Fixes: #10994

[AmitMY]

This looks good, however, using the name _hasRefresher implies private variable, and you are using it from refresher. Perhaps a better name would be hasRefresher

[Manduro]

My understanding from the Ionic source code is that properties that are prefixed with an underscore are for framework-internal use, and properties that are explicitly declared as private are for class-internal use. I'm sure the Ionic team can comment on this!

[AmitMY]

Did you try to do it only using CSS? (on .content.has-refresher)

Like:

.content.has-refresher .scroll-content, .content.has-refresher .fixed-content {
 top: -1px;
}

Or perhaps in refresher, there is this line:

content.setScrollElementStyle(Css.transform, ((y > 0) ? 'translateY(' + y + 'px) translateZ(0px)' : 'translateZ(0px)'));

You can try and change to

content.setScrollElementStyle(Css.transform, ((y > 0) ? 'translateY(' + y + 'px) translateZ(0px)' : 'translateY(-1px) translateZ(0px)'));

[manucorporat]

maybe this is a good oportunity to merge:

_content._hasRefresher = true;
_content.setElementClass('has-refresher', true);

to something like:

_content._setRefresher(true);

this way we reduce the coupling between both components a little bit, thoughts?

[Manduro]

@manucorporat Yes, that would be nice. Maybe we could use the following on Content and remove the setElementClass completely, so there's no need for a setter function either?

host {
  '[class.has-refresher]': '_hasRefresher'
}

[manucorporat]

@Manduro yes, prefixed with _ is the way most of the components are created. private instance variables can't be tested directly in unit tests and the angular compiler "crashes" when used in the template.

[manucorporat]

@AmitMY @Manduro I like the proposed CSS solution if it could be used without ANY additional JS code, otherwise I would say this solution is good enough

[AmitMY]

@manucorporat Yeah my suggestions are different. pure css one (I think it works), OR just a translation change in JS

[manucorporat]

@AmitMY not a big fan of the translation solution though

[Manduro]

Originally this was working with css only, a rule for this still exists:

.has-refresher .scroll-content {
  margin-top: -1px;
}

The problem with this is that it gets overwritten by Content's writeDimensions since some refactor back in the beta days. Using transform or relative positioning could very well generate other problems, so I think this simple js solution would be the best.

[AmitMY]

@Manduro but is top overwritten? Using JS just adds complexity to the component

[Manduro]

It is not as far as I know, but it could definitely introduce other problems with the absolute positioning which I can not oversee.

The scroll area would be 1px underneath the header as well, which is not the case with margins.

[manucorporat]

I can't prove it, but I do agree with @Manduro on this. Even if this adds small complexity in content:

1. The complexity was already there by calling content.setElementClass("has-refresher", true)
2. Content already has a well tested single point of management for the dynamic positioning, that uses margin.
3. I think it is better to rely only on margin-top than using mixed properties: margin and (top,translate) at the same time.

Also, I like the small refactor proposed:

host {
  '[class.has-refresher]': '_hasRefresher'
}

However, the change detection strategy used by Content is ChangeDetectionStrategy.OnPush, not sure if changing _hasRefresher can trigger a change detection by their own.

[Manduro]

@manucorporat Agreed on all three points!

I was just testing that host piece which works nicely on dev and I think it should work fine in prod with OnPush as well. Change detection is not involved afaik because the _hasRefresher property is set before render and never changes. The same is used for the statusbar-padding class.

Should I rewrite the property like below, or is it fine as it is?

/** @hidden */
hasRefresher: boolean;

[manucorporat]

@Manduro you right! it should not cause any problem

/** @internal */
_hasRefresher: boolean = false;

[Manduro]

@manucorporat Alright! I pushed the additional change, needs a squash on merge. Thanks for taking a look!

[manucorporat]

let's trust TS a little bit :D it should be a valid boolean. I think it is ok to drop the "=== true"

[manucorporat]

cc @Manduro

[manucorporat]

btw, I probably rename statusbarPadding to _statusbarPadding soon

[Manduro]

@manucorporat Haha, I like the explicitness and it could be a tiny bit faster but sure ;)

[manucorporat]

merged!