refactor: visibility observer #74
Conversation
chintankavathia
force-pushed
the
refactor/visibility-observer
branch
2 times, most recently
from
b130407
to
5268ad7
Compare
chintankavathia
force-pushed
the
refactor/visibility-observer
branch
from
5268ad7
to
6de5892
Compare
chintankavathia
force-pushed
the
refactor/visibility-observer
branch
from
6de5892
to
c665207
Compare
|
@timowolf Could you please review this? |
There was a problem hiding this comment.
@chintankavathia thanks a lot for the MR 🙇
Just one question from my end, the implementation itself looks a-ok from my end.
| /** | ||
| * Throttle time in ms. Will recalculate table this time after the resize. | ||
| */ | ||
| @Input() resizeThrottle = 100; | ||
|
|
||
| /** | ||
| * Emits first visibility change immediately without waiting for resizeThrottle. | ||
| */ | ||
| @Input() emitInitial = true; | ||
|
|
There was a problem hiding this comment.
Since the current/new behavior is not filled in within the MR template, would you mind commenting on why we should expose those values? I'm personally afraid that this just leads to non-aligned behavior across users and starts introducing issues. The input description also doesn't guide users to what values they should exactly use. I would therefore just pick sane defaults and not yet expose them to the end user, doing so only after we have a concrete need.
There was a problem hiding this comment.
@fh1ch currently we have siResizeObserver as part of our internal Element lib which is being used with datatable by consumers manually to trigger recalculate (people often forgets to do this and end up with issues). Goal of the MR is to remove that manual need however since siResizeObserver also allows these two props I am exposing the same here to keep it compatible with what we have today with siResizeObserver.
use `ResizeObserver` api to emit visibility change. Also auto recalculate table when table resize by parent containers.
chintankavathia
force-pushed
the
refactor/visibility-observer
branch
from
c665207
to
5111072
Compare
use
ResizeObserverapi to emit visibility change. Also auto recalculate table when table resize by parent containers.What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: