Skip to content

Conversation

@chmelevskij
Copy link

@chmelevskij chmelevskij commented Dec 2, 2025

SUMMARY

Added correct box-sizing for embedded dashboard elements. This fixes overlaps when embedding.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Screenshot 2025-12-02 at 10 45 21

After:
Screenshot 2025-12-02 at 10 43 57

TESTING INSTRUCTIONS

Embed one of the dashboards in external application.

ADDITIONAL INFORMATION

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Dec 2, 2025

Code Review Agent Run #899e24

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 4851b5e..4851b5e
    • superset-frontend/src/dashboard/containers/DashboardPage.tsx
    • superset-frontend/src/dashboard/styles.ts
  • Files skipped - 0
  • Tools
    • Eslint (Linter) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot bot added change:frontend Requires changing the frontend dashboard:css Related to the CSS field of the Dashboard labels Dec 2, 2025
@rusackas
Copy link
Member

rusackas commented Dec 2, 2025

That's a bit of a CSS shotgun, I'm just not sure if it won't have downstream effects where we were expecting things to be content-box for proper layout.

It might just work, but I wonder if there's not a better way to scope it to just the embedded SDK, or (maybe) be a little more surgical in the approach, adding it to the right components.

If we do go with this approach, we can probably remove a lot of border-box lines of styling throughout the codebase, but again, I'm just not sure where we want content-box

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 box-sizing issues in embedded dashboards that were causing layout overlaps. The fix applies the standard CSS box-sizing: border-box rule globally to ensure consistent element sizing.

  • Adds global box-sizing: border-box style to fix embedded dashboard layout overlaps
  • Integrates the new style into the dashboard's global style system

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
superset-frontend/src/dashboard/styles.ts Adds new boxSizing export with universal selector applying box-sizing: border-box
superset-frontend/src/dashboard/containers/DashboardPage.tsx Imports and applies the boxSizing style to the dashboard's global styles array

@rusackas
Copy link
Member

rusackas commented Dec 2, 2025

Just catching up on the thread in #36204 - that seems to have a more scoped approach to the styling, if it happens to work. ¯\_(ツ)_/¯

@chmelevskij
Copy link
Author

It does feel a bit aggressive, but reasoning behind this:

  1. It's quite common snippet in normalize/reset css scripts.
  2. Everything inside of ant design Layout already has this applied actually, and in most cases it takes priority over component level css due to specificity of the selectors.

Alternatives would be to put it either in DashboardBuilder styles OR using .dashboard class name in embedded stylings. Open to both if you think that's better option.

@rusackas
Copy link
Member

rusackas commented Dec 3, 2025

If that's true about using border-box in the same way, shouldn't that make things work already?

@rusackas
Copy link
Member

rusackas commented Dec 3, 2025

I'm just worried about breaking anything that might be using content-box. Times like this, I wish we had screenshot regression testing everywhere.

I think I'd be happy to merge this if the selector could be scoped down to be more specific, if indeed most of our stuff is within so we can be a bit more "surgical" (and performant) here.

@chmelevskij
Copy link
Author

If that's true about using border-box in the same way, shouldn't that make things work already?

I'm assuming that's a question about Layout. Ant design Layout is only used for the main application, but not for embedded.

I've looked if it's possible to add Layout but it doesn't seem to fit the usecase, or at least would need way much more rethinking to get it work.

I will update it to scope to .dashboar thne.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend dashboard:css Related to the CSS field of the Dashboard size/S

Projects

None yet

2 participants