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

Missing space between Dialog Header and Body #3043

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Missing space between Dialog Header and Body #3043

wants to merge 1 commit into from

Conversation

HDinger
Copy link
Contributor

@HDinger HDinger commented Aug 30, 2024

What are you trying to accomplish?

Add top-padding for the Dialog body to move it closer to the React component and avoid it being glued to the divider.

Screenshots

Before After
Default Bildschirmfoto 2024-08-30 um 14 43 59 Bildschirmfoto 2024-08-30 um 14 43 21
With large header and divider Bildschirmfoto 2024-08-30 um 14 44 29 Bildschirmfoto 2024-08-30 um 14 37 02
Custom header Bildschirmfoto 2024-08-30 um 14 45 33 Bildschirmfoto 2024-08-30 um 14 36 42
Header filter Bildschirmfoto 2024-08-30 um 14 45 12 Bildschirmfoto 2024-08-30 um 14 36 31

List the issues that this change affects.

Closes #3008

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

  • Looking at the React component, which always has a padding-top for the body of a dialog
  • In Rails this was explicitly canceled (probably because the divider is not always visible as in React??). However, this results in the body being glued to the header which looks super strange
  • I am aware that the spacings are sill not optimal and consistent with the React component, however I think that this might already be a good improvement and would fix the related issue.

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@HDinger HDinger requested review from a team as code owners August 30, 2024 12:53
@HDinger HDinger requested review from lukasoppermann and JoyceZhu and removed request for a team August 30, 2024 12:53
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2024

🦋 Changeset detected

Latest commit: d3e7ec9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions GitHub Actions
Copy link
Contributor

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

@github-actions github-actions bot added the Stale Automatically marked as stale. label Oct 29, 2024
@HDinger
Copy link
Contributor Author

HDinger commented Oct 29, 2024

Hi @team can I please get any feedback on this?

@github-actions github-actions bot removed the Stale Automatically marked as stale. label Oct 29, 2024
@lesliecdubs lesliecdubs removed the request for review from JoyceZhu November 6, 2024 03:31
lukasoppermann
lukasoppermann approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@lukasoppermann lukasoppermann left a comment

Choose a reason for hiding this comment

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

LGTM

@HDinger
Add top-padding for the Dialog Body to move closer to the React compo…
353ce46 
…nent and avoid it being glued to the divider
@HDinger HDinger force-pushed the fix/spacing-in-dialog branch from d3e7ec9 to 353ce46 Compare November 18, 2024 09:53
@HDinger
Copy link
Contributor Author

HDinger commented Nov 18, 2024

Thanks for the review @lukasoppermann ! 🙇 I think the CI failures are due to the fork setup. In previous PRs, a member of the Primer team opened an own PR with the commit to trigger the CI runs. Could someone please do that so that the visual regression step can run trough?

@HDinger
Copy link
Contributor Author

HDinger commented Dec 17, 2024

Hi everyone :) Are there any updates on this? Is there anything I can do to help? /cc @lukasoppermann @camertron @lesliecdubs

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

@lukasoppermann lukasoppermann lukasoppermann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dialog spacings between content and header in Rails
2 participants
@HDinger @lukasoppermann