Skip to content

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Mar 4, 2020

Closes https://jira.corp.adobe.com/browse/RSP-1530

Still not 100% why the heck this happens but I've adjusted the max width to accommodate for the checkbox's right padding and the scrolling issue is now gone.

✅ Pull Request Checklist:

  • Included link to corresponding Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Go to "dialog: form" story and make sure there isn't horizontal scrolling happening on medium or large scale. For a base reproduction of the old issue, reset the css changes here and go to the Checkbox story and change the render to look like this:

    <div style={{width: "200px", "flex-direction": "column", overflow: "auto", background: "gainsboro"}}>
      <Checkbox
        width="100%"
        onChange={action('change')}
        {...props}>
        Checkbox Label
      </Checkbox>
    </div>

min-block-size: var(--spectrum-checkbox-height);
max-inline-size: 100%;
/* Accommodates for right padding in case width: 100% is applied to the Checkbox directly. */
max-inline-size: calc(100% - var(--spectrum-checkbox-cursor-hit-x) * 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

???????????? Still digging to find out why this helps. At first I thought it was that the hit area was exceeding the container width and thus causing overflow to kick in but it doesn't truly seem to be the case.

Fix works tho

Copy link
Member

Choose a reason for hiding this comment

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

wha? but that element is absolutely positioned, it shouldn't affect the size at all
I do believe you that this works
did you try small sizes though? it may still fall apart there or the box size may looks funny for very wide cases (not that it would be bad in the wide case)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, its not the hit area, it is actually just the checkbox's margin-inline-end padding that causes the checkbox to exceed the container width when width: 100% is applied directly to the Checkbox. I tried doing box-sizing: border-box; as was suggested in some posts I read but no dice. I figured the above change was preferable to removing the margin-inline-end padding since I felt people were more likely to put things next to checkboxes and since the above change just shortens the hit box a little bit

At small sizes the shortened checkbox is more noticeable for sure, but not horribly so IMO, especially at those widths you are dealing with single letter labels (which are still clickable actually):
image
Uncaptured in the screenshot is my mouse hovering over the right edge of the "C" and it still changing into the hand(?) cursor that indicates a button/clickable element of some sort

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer having the box-sizing: border-box approach but can't for the life of me figure out why it isn't working

@adobe-bot
Copy link

Build successful! 🎉

ktabors
ktabors previously approved these changes Mar 4, 2020
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

LGTM

MidnightCoder06
MidnightCoder06 previously approved these changes Mar 5, 2020
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@LFDanLu LFDanLu dismissed stale reviews from MidnightCoder06 and ktabors via 7c14b7a March 10, 2020 17:47
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@dannify
Copy link
Member

dannify commented Mar 10, 2020

We will need to retest Checkboxes with this change

@dannify dannify merged commit 811d295 into master Mar 10, 2020
@dannify dannify deleted the RSP-1530 branch March 10, 2020 18:46
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.

6 participants