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.
There was a problem hiding this comment.
@chintankavathia thanks a lot for the reply 🙇
@spike-rabbit thanks also for your comment. I do agree that we should probably wait with this one a bit, so we can address the resize events holistically. To quickly outline my concerns with the current approach:
- Consumers had to handle the resize event on their own so far and trigger the recalculation "manually". If we now suddenly handle this internally we create two trigger sources, which can be problematic if we consider performance. I therefore think this should be treated as a breaking change, so consumers are aware of it and can remove the overhead code on their end.
- Even if the internal
siResizeObserverdirective did offer inputs likeresizeThrottleandemitInitial, I don't think that we need API compatibility since users anyway have to manually migrate (see point 1) and they also don't really make a lot of sense in the context of the datatable (especially the later).
Hence my point that we probably shouldn't integrate them from the get-go. Feel free to share your opinion here, but as said by @spike-rabbit, we can also discuss this once we rework the resize events all-together.
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
|
I would like to postpone this for a while. I see chances, that we may can get rid of listening to resize events completely. |
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: