Skip to content

fix(#3541) - adjust modal so left side of Checkbox is not cut off#3666

Open
mxsoco wants to merge 1 commit intodevfrom
mxsoco/3541-checkbox-modal-cut-off
Open

fix(#3541) - adjust modal so left side of Checkbox is not cut off#3666
mxsoco wants to merge 1 commit intodevfrom
mxsoco/3541-checkbox-modal-cut-off

Conversation

@mxsoco
Copy link
Collaborator

@mxsoco mxsoco commented Mar 24, 2026

Before (the change)

On focus, left side of the checkbox gets cut off by the modal container.
image

After (the change)

After adjusting the padding of the Modal and Scrollable container, the left side now displays on modal.
image

Steps needed to test

  • Ensure that v1 styles for modal remains the same
  • Confirm that checkbox left side does not get cut off by the side of the "scrollable" container.
  • Check if checkbox left side still appears when modal has a scrollbar.

@mxsoco mxsoco force-pushed the mxsoco/3541-checkbox-modal-cut-off branch 2 times, most recently from a05b652 to e0321f4 Compare March 24, 2026 18:45
@mxsoco mxsoco force-pushed the mxsoco/3541-checkbox-modal-cut-off branch from d8df608 to 83df705 Compare March 24, 2026 18:58
@mxsoco mxsoco requested review from bdfranck and chrisolsen March 24, 2026 19:51
direction="vertical"
hpadding="var(--scrollable-padding)"
hpadding={version === "2" ? "var(--goa-space-2xs)" : "var(--scrollable-padding)"}
vpadding={version === "2" ? "0" : ""}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense that the vpadding is either 0 or ""?

<goa-scrollable
direction="vertical"
hpadding="var(--scrollable-padding)"
hpadding={version === "2" ? "var(--goa-space-2xs)" : "var(--scrollable-padding)"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this override required? If things are version 2 then the --scrollable-padding should be overridden already.

Comment on lines +456 to +457
padding-left: var(--goa-space-s);
padding-right: var(--goa-space-s);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These padding values should just be moved into the padding above it.

Copy link
Collaborator

@bdfranck bdfranck left a comment

Choose a reason for hiding this comment

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

Instead of modifying the component itself, I suggest that we update the Modal design tokens.

When we move the left/right padding out of --goa-modal-content-padding...

  "modal-content-padding": {
    "value": "{space.l} 0 {space.xl} 0",
    "type": "spacing",
    "description": "Content section padding"
  },

...and into --goa-modal-scrollable-padding-desktop...

  "modal-scrollable-padding-desktop": {
    "value": "{space.l}",
    "type": "spacing",
    "description": "Scrollable component horizontal padding for desktop"
  },

...and do the same for the mobile versions...

  "modal-content-padding-mobile": {
    "value": "{space.l} 0 {space.xl} 0",
    "type": "spacing",
    "description": "Content section padding for mobile (24px top, 16px sides, 32px bottom)"
  },
  "modal-scrollable-padding-mobile": {
    "value": "{space.m}",
    "type": "spacing",
    "description": "Scrollable component horizontal padding for mobile"
  },

...then we can achieve the same result without altering the component itself.

Image

<goa-scrollable
direction="vertical"
hpadding="var(--scrollable-padding)"
hpadding={version === "2" ? "var(--goa-space-2xs)" : "var(--scrollable-padding)"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to keep --scrollable-padding here. It's being used for responsive padding.

direction="vertical"
hpadding="var(--scrollable-padding)"
hpadding={version === "2" ? "var(--goa-space-2xs)" : "var(--scrollable-padding)"}
vpadding={version === "2" ? "0" : ""}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this line do?


.v2 .modal-content {
padding: var(--goa-modal-content-padding-mobile, var(--goa-space-l) var(--goa-space-m) var(--goa-space-xl) var(--goa-space-m));
padding-left: var(--goa-space-s);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update the --goa-modal-content-padding-mobile design token's values instead of overriding it with a static value.

box-shadow: none;
}

.v2 .modal-content {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update --goa-modal-content-padding instead.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants