Scheduler — Replace underscore-prefixed: m_tooltip_strategy_base.ts, m_work_space.ts#32962
Conversation
… and m_work_space.ts
aleksei-semikozov
changed the title
There was a problem hiding this comment.
Pull request overview
This PR modernizes internal Scheduler code by replacing several underscore-prefixed instance fields/methods with non-underscore equivalents (and adjusting visibility where appropriate), keeping related unit tests in sync.
Changes:
- Updated Scheduler workspace internals to use non-underscore field names (and renamed some DOM helper methods).
- Updated tooltip strategy internals to expose non-underscore
tooltip/list/extraOptionsfields for derived strategies/tests. - Adjusted Scheduler QUnit tests to reference the renamed fields.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.scheduler/workSpace.renovation.tests.js | Updates workspace test to use $allDayTable instead of _$allDayTable. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.scheduler/keyboardNavigation.tests.js | Updates tooltip test to use appointmentTooltip.list instead of appointmentTooltip._list. |
| packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts | Refactors multiple workspace fields/methods to non-underscore names; updates call sites accordingly. |
| packages/devextreme/js/__internal/scheduler/workspaces/m_agenda.ts | Aligns agenda workspace DOM fields with the renamed base workspace fields. |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_tooltip_strategy_base.ts | Refactors tooltip strategy base fields to tooltip/list/extraOptions and updates internal usage. |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_mobile_tooltip_strategy.ts | Updates to use this.list / this.tooltip. |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_desktop_tooltip_strategy.ts | Updates to use this.tooltip / this.list / this.extraOptions. |
You can also share your feedback on Copilot code review. Take the survey.
packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts
Outdated
Show resolved
Hide resolved
| _cellsSelectionController: any; | ||
| private cellsSelectionControllerValue: any; | ||
|
|
||
| // TODO: used externally by shaders, grouped strategies, m_agenda.ts |
There was a problem hiding this comment.
Do you think this comment clearly explains the goal of what needs to be done?
There was a problem hiding this comment.
What would you suggest instead? Something like
// TODO: make private once external usages in shaders, grouped strategies, m_agenda.ts are removed?
There was a problem hiding this comment.
Yes, something like that works
| private isOutsideScrollable(target, event) { | ||
| const $dateTableScrollableElement = this._dateTableScrollable.$element(); | ||
| const scrollableSize = getBoundingRect($dateTableScrollableElement.get(0)); | ||
| const $_dateTableScrollableElement = this._dateTableScrollable.$element(); |
There was a problem hiding this comment.
Did I understand correctly that you renamed dateTableScrollableElement to _dateTableScrollableElement because of _dateTableScrollable? If we want to rename this variable again after renaming of _dateTableScrollable we better add todo comment here. Or maybe we could cancel this renaming because that _dateTableScrollable must be renamed in the future.
There was a problem hiding this comment.
Yes, we introduced $_dateTableScrollableElement alongside $_dateTableScrollable by mistake. Both were renamed back to $dateTableScrollable / $dateTableScrollableElement in the follow-up commit (9b2a01c), so local variables no longer have the underscore prefix.
There was a problem hiding this comment.
Pull request overview
This PR continues the Scheduler internal refactoring by replacing underscore-prefixed instance fields/members in core workspace and tooltip strategy code, updating related workspace implementations and tests to use the new member names.
Changes:
- Refactor
SchedulerWorkSpaceinternals to use non-underscore private/protected fields (plus a few explicitly retained underscored members with TODO notes for external usages). - Refactor
TooltipStrategyBase(and desktop/mobile strategies) to use non-underscore protected fields and update call sites. - Update Scheduler QUnit tests that accessed the renamed internal fields.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.scheduler/workSpace.renovation.tests.js | Updates renovated workspace test to check $allDayTable instead of _$allDayTable. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.scheduler/keyboardNavigation.tests.js | Updates tooltip list access from _list to list. |
| packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts | Replaces many underscore-prefixed fields with private/protected equivalents and updates internal usages accordingly. |
| packages/devextreme/js/__internal/scheduler/workspaces/m_agenda.ts | Aligns agenda workspace implementation with renamed workspace fields ($timePanel, $dateTableContainer). |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_tooltip_strategy_base.ts | Renames _tooltip/_list/_extraOptions to tooltip/list/extraOptions and updates base strategy logic. |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_mobile_tooltip_strategy.ts | Updates mobile tooltip strategy to use renamed tooltip/list fields. |
| packages/devextreme/js/__internal/scheduler/tooltip_strategies/m_desktop_tooltip_strategy.ts | Updates desktop tooltip strategy to use renamed tooltip/list/extraOptions fields. |
You can also share your feedback on Copilot code review. Take the survey.
No description provided.