Popup: Remove dx-toolbar by marker-dao · Pull Request #32686 · DevExpress/DevExtreme

marker-dao · 2026-02-24T16:20:07Z

No description provided.

Copilot

Pull request overview

This pull request aims to remove the dx-toolbar CSS class from the Popup component's title and bottom toolbar elements. However, the implementation appears incomplete and introduces several critical issues that prevent it from being production-ready.

Changes:

Commented out the TOOLBAR_CLASS import and its usage in popup toolbar rendering
Modified template wrapper class handling logic to conditionally move the dx-template-wrapper class between elements
Updated all framework demos (jQuery, React, Vue, Angular) to test title templates with visible: true by default
Added CSS styling for .title class in demo files

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 18 comments.

Show a summary per file

File	Description
packages/devextreme/js/__internal/ui/popup/m_popup.ts	Core changes: commented out TOOLBAR_CLASS usage, modified template wrapper logic, added ESLint disable comments
packages/devextreme/playground/jquery.html	Added test case for popup with title template and toolbar items
apps/demos/Demos/Popup/Overview/jQuery/index.js	Removed employee content display, simplified to test title template
apps/demos/Demos/Popup/Overview/jQuery/index.html	Added title template markup
apps/demos/Demos/Popup/Overview/jQuery/styles.css	Added `.title` class styling
apps/demos/Demos/Popup/Overview/Vue/App.vue	Changed popup to visible=true, replaced content with title template
apps/demos/Demos/Popup/Overview/React/App.tsx	Changed popup to visible=true, commented out functions, replaced content with title template
apps/demos/Demos/Popup/Overview/React/styles.css	Added `.title` class styling
apps/demos/Demos/Popup/Overview/Angular/app/app.component.ts	Changed popupVisible to true
apps/demos/Demos/Popup/Overview/Angular/app/app.component.html	Replaced content template with title template
apps/demos/Demos/Popup/Overview/Angular/app/app.component.css	Added `.title` class styling with `::ng-deep`

Comments suppressed due to low confidence (1)

apps/demos/Demos/Popup/Overview/jQuery/index.js:42

The demo has been modified to remove the employee information display functionality (contentTemplate and employee details). The popup is now visible by default and no longer shows employee data when Details buttons are clicked. This appears to be for testing the toolbar/title changes, but it breaks the demo's intended functionality. If this is temporary for testing, it should not be merged. If it's intentional, the demo needs to show a proper use case for the Popup component.

  const popup = $('#popup').dxPopup({
    width: 450,
    height: 600,
    container: '.dx-viewport',
    visible: true,
    dragEnabled: false,
    toolbarItems: [{
      widget: 'dxButton',
      toolbar: 'bottom',
      location: 'before',
      options: {
        icon: 'email',
        stylingMode: 'contained',
        text: 'Send',
        onClick() {
          const message = `Email is sent to ${employee.FirstName} ${employee.LastName}`;
          DevExpress.ui.notify({
            message,
            position: {
              my: 'center top',
              at: 'center top',
            },
          }, 'success', 3000);
        },
      },
    }, {
      widget: 'dxButton',
      toolbar: 'bottom',
      location: 'after',
      options: {
        text: 'Close',
        stylingMode: 'outlined',
        type: 'normal',
        onClick() {
          popup.hide();
        },
      },
    }],
  }).dxPopup('instance');

Copilot · 2026-02-24T17:21:08Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

 } from '@ts/ui/overlay/overlay_position_controller';
 import * as zIndexPool from '@ts/ui/overlay/z_index';
-import { TOOLBAR_CLASS } from '@ts/ui/toolbar/constants';
+// import { TOOLBAR_CLASS } from '@ts/ui/toolbar/constants';


The commented-out import of TOOLBAR_CLASS should be removed entirely rather than left as a comment. Commented-out imports create confusion about whether the dependency is still needed and clutter the codebase. Since the class is no longer being used (as indicated by the commented-out addClass calls below), this import should be deleted.

Suggested change

// import { TOOLBAR_CLASS } from '@ts/ui/toolbar/constants';

Copilot · 2026-02-24T17:21:09Z

apps/demos/Demos/Popup/Overview/Vue/App.vue


 const currentEmployee = ref<Employee>();
-const popupVisible = ref(false);
+const popupVisible = ref(true);


The popup's visible property is set to true, causing it to display immediately on page load. This breaks the demo's intended user interaction flow where users click "Details" buttons to show employee information. The popup should start as visible: false to maintain the proper demo behavior.

Suggested change

const popupVisible = ref(true);

const popupVisible = ref(false);

Copilot · 2026-02-24T17:21:09Z

packages/devextreme/playground/jquery.html

+                const popup = $("#popup")
+                    .dxPopup({
+                        visible: true,
+                        dragEnabled: false,
+                        width: 450,
+                        height: 600,
+                        toolbarItems: [
+                            {
+                                widget: "dxButton",
+                                toolbar: "bottom",
+                                location: "before",
+                                options: getEmailButtonOptions()
+                            },
+                            {
+                                widget: "dxButton",
+                                toolbar: "bottom",
+                                location: "before",
+                                options: getEmailButtonOptions()
+                            }
+                        ]
+                    })
+                    .dxPopup("instance");


The playground file appears to be a testing/development file that has been modified to test the toolbar title template functionality. The changes show duplicate button configurations (both using getEmailButtonOptions) which doesn't demonstrate varied functionality. While this might be for testing purposes, consider using more meaningful test cases that show different button configurations to better validate the changes.

Copilot · 2026-02-24T17:21:09Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

        index++;
      }

+      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing


The ESLint disable comment for prefer-nullish-coalescing should include an explanation of why the logical OR operator is preferred over nullish coalescing in this specific context. Without explanation, it's unclear whether this is intentional or if the code should use the ?? operator instead.

Suggested change

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- toolbar should fall back to 'top' for any falsy value (e.g., empty string), so logical OR is intentional

Copilot · 2026-02-24T17:21:10Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

    const $toolbarContainer = $('<div>')
      .addClass(POPUP_TITLE_CLASS)
-      .addClass(TOOLBAR_CLASS)
+      // .addClass(TOOLBAR_CLASS)


The commented-out addClass(TOOLBAR_CLASS) call should be removed entirely. Since TOOLBAR_CLASS is no longer being used (as evidenced by the commented import), this commented code serves no purpose and should be deleted. However, this removal may be a breaking change as SCSS styles specifically target .dx-popup-title.dx-toolbar (see packages/devextreme-scss/scss/widgets/generic/popup/_index.scss lines 51-63). Verify that the styling still works correctly without the dx-toolbar class.

Copilot · 2026-02-24T17:21:12Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

-    if ($result.hasClass(TEMPLATE_WRAPPER_CLASS)) {
+    const resultClassList = Array.from($result.get(0)?.classList ?? []);
+
+    // const hasResultTemplateWrapperClass = $result.hasClass(TEMPLATE_WRAPPER_CLASS);


This commented-out code block with jQuery hasClass() call should be removed. The new implementation using Array.from and includes() already replaces this functionality. Leaving commented code creates confusion and makes the codebase harder to maintain.

Copilot · 2026-02-24T17:21:12Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

+    // // eslint-disable-next-line no-debugger
+    // debugger;


Commented-out debugger statements should be removed entirely. These debugging artifacts should not be committed to the codebase.

Copilot · 2026-02-24T17:21:12Z

packages/devextreme/js/__internal/ui/popup/m_popup.ts

+    // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
    }, data.options || {});


The ESLint disable comment for prefer-nullish-coalescing should include an explanation of why the logical OR operator is preferred over nullish coalescing in this specific context. Without explanation, it's unclear whether this is intentional or if the code should use the ?? operator instead.

Suggested change

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing

}, data.options || {});

}, data.options ?? {});

Copilot · 2026-02-24T17:21:12Z

apps/demos/Demos/Popup/Overview/React/App.tsx

+  // const showMoreInfo = useCallback(() => {
+  //   const message = `More info about ${currentEmployee?.FirstName} ${currentEmployee?.LastName}`;
+  //   notify(
+  //     {
+  //       message,
+  //       position: {
+  //         my: 'center top',
+  //         at: 'center top',
+  //       },
+  //     },
+  //     'success',
+  //     3000,
+  //   );
+  // }, [currentEmployee]);

-  const getInfoButtonOptions = useMemo((): ButtonTypes.Properties => ({
-    text: 'More info',
-    onClick: showMoreInfo,
-  }), [showMoreInfo]);
+  // const getInfoButtonOptions = useMemo((): ButtonTypes.Properties => ({
+  //   text: 'More info',
+  //   onClick: showMoreInfo,
+  // }), [showMoreInfo]);


The commented-out showMoreInfo function and related code should be removed entirely rather than left as comments. Commented-out code creates confusion about whether it might be needed in the future and makes the codebase harder to maintain. If this functionality is no longer needed, delete it. If it might be needed later, version control history will preserve it.

Copilot · 2026-02-24T17:21:13Z

apps/demos/Demos/Popup/Overview/React/App.tsx

      <Popup
        visible={popupVisible}
        onHiding={hideInfo}
        dragEnabled={false}
-        hideOnOutsideClick={true}
-        showCloseButton={false}
-        showTitle={true}
-        title="Information"
-        container=".dx-viewport"
-        width={300}
-        height={280}
+        width={450}
+        height={600}
+        titleRender={renderTitle}
      >
-        <Position at="bottom" my="center" of={positionOf} collision="fit" />
-        <ToolbarItem
-          widget="dxButton"
-          toolbar="top"
-          locateInMenu="always"
-          options={getInfoButtonOptions}
-        />
        <ToolbarItem
          widget="dxButton"
          toolbar="bottom"
          location="before"
          options={getEmailButtonOptions}
        />
        <ToolbarItem
          widget="dxButton"
          toolbar="bottom"
          location="after"
          options={getCloseButtonOptions}
        />
-        <p>
-          Full Name:&nbsp;
-          <span>{currentEmployee?.FirstName}</span>&nbsp;
-          <span>{currentEmployee?.LastName}</span>
-        </p>
-        <p>
-          Birth Date: <span>{currentEmployee?.BirthDate}</span>
-        </p>
-        <p>
-          Address: <span>{currentEmployee?.Address}</span>
-        </p>
-        <p>
-          Hire Date: <span>{currentEmployee?.HireDate}</span>
-        </p>
-        <p>
-          Position: <span>{currentEmployee?.Position}</span>
-        </p>
      </Popup>


The demo has been modified to remove the employee information display and position configuration. The popup now only shows a title template instead of employee details when users click "Details" buttons. This fundamentally changes what the demo demonstrates and doesn't align with a typical "Overview" demo that should showcase various Popup features including content display and positioning.

marker dao ® added 2 commits February 24, 2026 16:38

refactor(m_popup.ts): Remove alerts

280bdc6

Popup: Remove dx-toolbar

91b2be5

marker-dao self-assigned this Feb 24, 2026

marker-dao added skip-cache force all tests 26_1 labels Feb 24, 2026

fix(m_popup.ts): Get rid of replacing in Vue

f4aa876

marker-dao marked this pull request as ready for review February 24, 2026 17:15

marker-dao requested a review from a team as a code owner February 24, 2026 17:15

Copilot AI review requested due to automatic review settings February 24, 2026 17:15

Copilot started reviewing on behalf of marker-dao February 24, 2026 17:16 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Popup: Remove dx-toolbar#32686

Popup: Remove dx-toolbar#32686
marker-dao wants to merge 3 commits intoDevExpress:26_1from
marker-dao:26_1_T1322170_popup_title

marker-dao commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	const popupVisible = ref(true);
	const popupVisible = ref(false);

	// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
	// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- toolbar should fall back to 'top' for any falsy value (e.g., empty string), so logical OR is intentional

		// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
		}, data.options \|\| {});

	// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
	}, data.options \|\| {});
	}, data.options ?? {});

Comments

Conversation

marker-dao commented Feb 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant