-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix Dialog grid iphone #3023
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.
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
Fix Dialog grid iphone #3023
Conversation
|
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these lines?
react-spectrum/packages/@adobe/spectrum-css-temp/components/dialog/index.css
Lines 393 to 433 in 6510792
| .spectrum-Dialog--fullscreen, | |
| .spectrum-Dialog--fullscreenTakeover { | |
| &.spectrum-Dialog .spectrum-Dialog-grid { | |
| display: grid; | |
| grid-template-columns: var(--spectrum-dialog-padding-x) 1fr var(--spectrum-dialog-padding-x); | |
| grid-template-rows: var(--spectrum-dialog-padding-y) auto auto auto 1fr auto var(--spectrum-dialog-padding-y); | |
| grid-template-areas: | |
| ". . ." | |
| ". heading ." | |
| ". header ." | |
| ". divider ." | |
| ". content ." | |
| ". buttonGroup ." | |
| ". . ."; | |
| } | |
| .spectrum-Dialog-heading { | |
| &.spectrum-Dialog-heading--noHeader, | |
| &.spectrum-Dialog-heading--noTypeIcon { | |
| grid-area: heading; | |
| } | |
| /* there will never be a typeIcon here */ | |
| &.spectrum-Dialog-heading--noHeader.spectrum-Dialog-heading--noTypeIcon { | |
| grid-area: heading; | |
| } | |
| } | |
| .spectrum-Dialog-header { | |
| &.spectrum-Dialog-header--noTypeIcon { | |
| grid-area: header; | |
| } | |
| } | |
| .spectrum-Dialog-buttonGroup { | |
| padding-block-start: var(--spectrum-global-dimension-static-size-500); | |
| } | |
| .spectrum-Dialog-heading { | |
| font-size: var(--spectrum-dialog-title-text-size); | |
| } | |
| } |
They exist for 400% which your media query satisfies, but they might be useful for edge cases like a tall skinny browser inside a desktop application like photoshop.
|
I think they should stay, it's really about the limited width, mine just makes sure the height one doesn't override the width one |
reidbarber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes

Found during testing
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: