Skip to content
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

SQFormDialog content body margin #88

Closed
SeanGroff opened this issue Feb 4, 2021 · 2 comments · Fixed by #310
Closed

SQFormDialog content body margin #88

SeanGroff opened this issue Feb 4, 2021 · 2 comments · Fixed by #310
Assignees
@Chris-Boyle
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@SeanGroff
Copy link
Contributor

SeanGroff commented Feb 4, 2021 •
edited
Loading

The SQDialog Modal needs specific interior margins set by default.
Interior Content Margins of the scrollable content area needs to be 20PX on the top, left, bottom, and right sides of the content area.

UPDATE: As of 6/21/2021, Nemeth has clarified we should change the existing padding: 16px 24px of the DialogContent component to be padding: 20px (20 pixels of padding for the Top, Left, Bottom, and Right)

This should be a BREAKING change as consumers who previously added a div to control the padding will need to fix their existing implementation.

Comp: https://www.sketch.com/s/b1684560-808a-4f20-960d-fd3969080240/a/GmoOLYY#Inspector
@SeanGroff SeanGroff added good first issue Good for newcomers help wanted Extra attention is needed labels Feb 4, 2021
@SeanGroff SeanGroff changed the title SQDialog content body margin SQFormDialog content body margin Feb 4, 2021
@20BBrown14 20BBrown14 self-assigned this Feb 26, 2021
@20BBrown14
Copy link
Contributor

Looking at this it seems that the content already has margins for the "scrollable" area. Or perhaps I'm missing something? Screenshots below from storybook. Although not of the same size asked for here.

Screen Shot 2021-02-26 at 11 02 19 AM

Screen Shot 2021-02-26 at 11 02 13 AM

Screen Shot 2021-02-26 at 11 01 33 AM

Screen Shot 2021-02-26 at 11 01 27 AM

@20BBrown14 20BBrown14 added question Further information is requested and removed in progress labels Feb 26, 2021
@20BBrown14 20BBrown14 removed their assignment Jun 21, 2021
@SeanGroff SeanGroff removed the question Further information is requested label Jun 21, 2021
@Chris-Boyle Chris-Boyle self-assigned this Jun 21, 2021
@Chris-Boyle
Copy link
Contributor

The current padding is 8px, 24px
image
I will set a padding style on the DialogContent to have the padding be 20px on all sides
<DialogContent style={{overflowY: 'visible', padding: '20px'}}>

Chris-Boyle added a commit that referenced this issue Jun 21, 2021
@Chris-Boyle
feat: 🎸 adding 20px padding to dialogcontent
c4d589d 
adding 20px padding to dialogcontent

BREAKING CHANGE: 🧨 yes

✅ Closes: #88
@Chris-Boyle Chris-Boyle mentioned this issue Jun 21, 2021
Merged
Chris-Boyle added a commit that referenced this issue Jun 21, 2021
@Chris-Boyle
feat: 🎸 Change SQFormDialog content padding to 20px
783194a 
Changed SQFormDialog content padding to 20px

BREAKING CHANGE: 🧨 Changed SQFormDialog content padding to 20px from 8px 24px

✅ Closes: #88
Chris-Boyle added a commit that referenced this issue Jun 22, 2021
@Chris-Boyle
feat: 🎸 Adding 20px padding to dialog content
24df034 
BREAKING CHANGE: 🧨 Adding 20px padding to SQFormDialog content instead of 8px 24px

✅ Closes: #88
SeanGroff pushed a commit that referenced this issue Jun 22, 2021
@semantic-release-bot
chore(release): 5.0.0 [skip ci]
99bff0e 
## [5.0.0](v4.9.0...v5.0.0) (2021-06-22)

### ⚠ BREAKING CHANGES

* 🧨 Adding 20px padding to SQFormDialog content instead of 8px 24px

### Features

* 🎸 Adding 20px padding to dialog content ([24df034](24df034)), closes [#88](#88)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Chris-Boyle Chris-Boyle

Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
3 participants
@SeanGroff @Chris-Boyle @20BBrown14