Skip to content

cc-dialog: fix close button positioning #1642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
florian-sanders-cc merged 2 commits into from
Jan 13, 2026
Merged

cc-dialog: fix close button positioning #1642

florian-sanders-cc merged 2 commits into from
Jan 13, 2026

Conversation

@florian-sanders-cc
Copy link
Contributor

@florian-sanders-cc florian-sanders-cc commented Jan 6, 2026 •
edited
Loading

Fixes #1637

What does this PR do?

  • Fixes the close button positioning when screen is narrow,
  • Fixes --cc-dialog-padding that was not doing what it was supposed to 🙈.

How to review?

  • Check the visual tests report or the stories (there's a slight difference in how the button is positionned even in desktop),
  • Try setting a different value for --cc-dialog-padding on the cc-dialog element or its parents.
    • The close button positioning is tricky, I wonder if we should expose a way to modify it directly instead of computing it like that, I don't really like how it's currently done.
  • 2 reviewers should be enough for this one but it would be nice to have @roberttran-cc 's opinion about the way we position the close button (if we don't have a definite solution right now, it can be merged as is and reworked later on in sync).

@florian-sanders-cc florian-sanders-cc self-assigned this Jan 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2026

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-dialog/fix-responsive-close-btn/index.html.

This preview will be deleted once this PR is closed.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2026 •
edited
Loading

🧐 Visual tests report for PR #1642

The latest visual tests report is available. Please review the results.

6 components impacted
  • cc-addon-info,
  • cc-domain-management,
  • cc-dialog-confirm-form,
  • cc-dialog,
  • cc-dialog-confirm-actions,
  • cc-addon-backups,

This comment was generated automatically by the Visual tests workflow.

roberttran-cc
roberttran-cc approved these changes Jan 7, 2026
View reviewed changes
Copy link
Member

@roberttran-cc roberttran-cc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

On the current fix, LGTM, works well now.

On the overall implementation, I think the exposed custom props are good as is: we hide the complexity and I prefer that (without further insights).

We can deal with the internal complexity for now and maybe find a cleaner way later (apart from changing the layout, I do not have a better solution in mind, except maybe some grid shenanigans).

edit: shootout to the visual diff, so nice to have!

pdesoyres-cc
pdesoyres-cc approved these changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

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

It works fine now. well done 👍

@florian-sanders-cc florian-sanders-cc force-pushed the cc-dialog/fix-responsive-close-btn branch from 3109141 to 385a4f6 Compare January 13, 2026 10:40
@florian-sanders-cc
fix(cc-dialog): make --cc-dialog-padding actually work
85d5ae6 
The internal variable was using the same name as the public CSS custom
property, causing user-provided values to be overridden. Renamed the
internal variable to --default-dialog-padding so it's used as a fallback.
@florian-sanders-cc
fix(cc-dialog): make close button position responsive on narrow screens
eed38b7 
The close button position is now derived from the dialog padding,
ensuring it stays properly positioned when the padding changes on
narrow screens.
@florian-sanders-cc florian-sanders-cc force-pushed the cc-dialog/fix-responsive-close-btn branch from 385a4f6 to eed38b7 Compare January 13, 2026 10:53
@florian-sanders-cc florian-sanders-cc merged commit ea65fbd into master Jan 13, 2026
15 checks passed
@florian-sanders-cc florian-sanders-cc deleted the cc-dialog/fix-responsive-close-btn branch January 13, 2026 11:00
@github-actions
Copy link
Contributor

github-actions bot commented Jan 13, 2026

🔎 The preview has been automatically deleted.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 13, 2026

🧹 Visual Changes Report deleted

The report and its associated data have been deleted because this PR has been closed.

This comment was generated automatically by the Visual Changes workflow.

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

Reviewers

@roberttran-cc roberttran-cc roberttran-cc approved these changes

@pdesoyres-cc pdesoyres-cc pdesoyres-cc approved these changes

@hsablonniere hsablonniere Awaiting requested review from hsablonniere

@Galimede Galimede Awaiting requested review from Galimede

@HeleneAmouzou HeleneAmouzou Awaiting requested review from HeleneAmouzou

@clara-layus-cc clara-layus-cc Awaiting requested review from clara-layus-cc

Assignees

@florian-sanders-cc florian-sanders-cc

Labels

run-visual-tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cc-dialog: close button is too close to the edge when spacing is reduced

4 participants

@florian-sanders-cc @roberttran-cc @pdesoyres-cc