refactor: extract and unify dialog bounds handling logic #9486
Conversation
web-padawan
force-pushed
the
refactor/dialog-set-bounds
branch
from
e71b77c to
f361491
Compare
web-padawan
force-pushed
the
refactor/dialog-set-bounds
branch
from
7f85f1e to
83f89a0
Compare
|
|
Proposed fix is to set |
| this._originalMouseCoords = {}; | ||
|
|
||
| // Get or create an instance of manager | ||
| this.__manager = DialogManager.create(this); |
There was a problem hiding this comment.
I'm not sure if this an improvement in terms of structure and readability. Some (very small) parts of the drag and resize logic are now "hidden" in the manager class, while the majority of it stays in the mixins. The class itself doesn't encapsulate any logic on its own, but just contains some utilities and some state. Its purpose is not clear from the name and it's also hard to come up with a better one. The weak map was also confusing me at first.
I think I prefer the duplication in this case, makes it easier to understand what's going on. As an alternative, a random idea would be to merge the two mixins. Or extract some utility functions that avoid duplicating some of the calculations at least. But I don't see a lot of value here in general, unless I'm missing some context where we would need to introduce more duplication for a new feature.
There was a problem hiding this comment.
Thanks, actually I was also thinking about merging mixins. It's probably ok to keep the code the way it is.
I still think we should revert some changes from the base styles PR and update tests accordingly.
Let's create a separate PR with just that change applied to existing core styles. |
|
I updated #9438 with proposed fix. Could be picked into core styles as well, but I don't know if that’s absolutely necessary. |
Description
Extracted from #9438, see #9438 (comment) and #9438 (comment).
Currently, we have
_originalBoundsand_originalMouseCoordsmostly duplicated in two mixins due to how dialog is structured. Refactored most of the logic to be placed in separate helper class (could be also a controller but it doesn't really have to do anything with connected / disconnected / updated callbacks, so I just used a plain class).Note: original change (first commit in this PR) also reverted #7970 and #8830 which would re-introduce #436 and #8790 respectively. In the last commit, I reverted that behavior and restored corresponding tests.
Type of change