Skip to content

Conversation

@sjbur
Copy link
Contributor

@sjbur sjbur commented Nov 25, 2025

No description provided.

@sjbur sjbur self-assigned this Nov 25, 2025
@sjbur sjbur added the 25_2 label Nov 25, 2025
@sjbur sjbur marked this pull request as ready for review November 26, 2025 10:55
@sjbur sjbur requested a review from a team as a code owner November 26, 2025 10:55
Copilot AI review requested due to automatic review settings November 26, 2025 10:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where custom width and maxWidth options specified in editing.popup were not being applied to the Scheduler's appointment popup. The fix stores these custom values and applies them dynamically in the updatePopupFullScreenMode() method, which is called when the popup is shown or when the window is resized.

Key Changes:

  • Added storage for custom width and maxWidth values from editing.popup configuration
  • Modified updatePopupFullScreenMode() to apply custom dimensions, with special handling for fullScreen mode

this.popup.option('maxWidth', isFullScreen ? '100%' : maxWidth);

if (this.customWidth !== undefined) {
this.popup.option('width', this.customWidth);
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The width option should be conditional based on isFullScreen, similar to how maxWidth is handled below. When the popup is in fullScreen mode, the popup component internally sets width: '100%' (see _renderDimensions in the Popup component), so setting a custom width would be overridden and could cause confusion. Consider applying the same pattern:

if (this.customWidth !== undefined) {
  this.popup.option('width', isFullScreen ? '100%' : this.customWidth);
}
Suggested change
this.popup.option('width', this.customWidth);
this.popup.option('width', isFullScreen ? '100%' : this.customWidth);

Copilot uses AI. Check for mistakes.
Comment on lines 107 to 108
this.customWidth = customPopupOptions.width;
this.customMaxWidth = customPopupOptions.maxWidth;
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new width and maxWidth customization functionality lacks test coverage. Consider adding tests similar to the existing test at line 1454 ('should pass custom popup options from editing.popup to appointment popup') to verify:

  1. Custom width is applied when specified in editing.popup.width
  2. Custom maxWidth is applied when specified in editing.popup.maxWidth
  3. The behavior is correct when updatePopupFullScreenMode() is called
  4. Both properties work correctly in fullScreen and non-fullScreen modes

Copilot uses AI. Check for mistakes.
@Tucchhaa Tucchhaa self-requested a review November 26, 2025 11:18
Copilot AI review requested due to automatic review settings November 27, 2025 09:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Tucchhaa
Tucchhaa previously approved these changes Nov 27, 2025
afterEach(() => {
Object.defineProperty(document.documentElement, 'clientWidth', {
writable: true,
configurable: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In after, we set this value to the same as in before. Remove, or set it diff state
writable props don't have affect

Copilot AI review requested due to automatic review settings November 27, 2025 14:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines +2187 to +2193
// Mock window width to avoid fullscreen mode
beforeEach(() => {
Object.defineProperty(document.documentElement, 'clientWidth', {
value: 1280,
});
});

Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Object.defineProperty call should include configurable: true to allow cleanup and make the property properly overridable. Additionally, consider adding an afterEach hook to restore the original clientWidth value to prevent side effects on other tests.

beforeEach(() => {
  Object.defineProperty(document.documentElement, 'clientWidth', {
    value: 1280,
    configurable: true, // Add this
  });
});
Suggested change
// Mock window width to avoid fullscreen mode
beforeEach(() => {
Object.defineProperty(document.documentElement, 'clientWidth', {
value: 1280,
});
});
// Mock window width to avoid fullscreen mode
let originalClientWidthDescriptor: PropertyDescriptor | undefined;
beforeEach(() => {
// Save the original descriptor so we can restore it after each test
originalClientWidthDescriptor = Object.getOwnPropertyDescriptor(document.documentElement, 'clientWidth');
Object.defineProperty(document.documentElement, 'clientWidth', {
value: 1280,
configurable: true,
});
});
afterEach(() => {
if (originalClientWidthDescriptor) {
Object.defineProperty(document.documentElement, 'clientWidth', originalClientWidthDescriptor);
} else {
// If there was no original descriptor, remove the property
delete (document.documentElement as any).clientWidth;
}
});

Copilot uses AI. Check for mistakes.
@sjbur sjbur merged commit 7df5f69 into DevExpress:25_2 Nov 27, 2025
97 of 99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants