view_components icon indicating copy to clipboard operation
view_components copied to clipboard

Missing space between Dialog Header and Body

Open HDinger opened this issue 1 year ago β€’ 1 comments

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)
  • [x] Tested in Chrome
  • [x] Tested in Firefox
  • [x] Tested in Safari
  • [x] 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 avatar Aug 30 '24 12:08 HDinger

πŸ¦‹ Changeset detected

Latest commit: d3e7ec9152bb2d29bb39d240fce5f5ab20a0ca1f

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

changeset-bot[bot] avatar Aug 30 '24 12:08 changeset-bot[bot]

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[bot] avatar Oct 29 '24 13:10 github-actions[bot]

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

HDinger avatar Oct 29 '24 13:10 HDinger

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 avatar Nov 18 '24 10:11 HDinger

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

HDinger avatar Dec 17 '24 06:12 HDinger

Hi @HDinger, I'm sorry for the delay! I'll add this to the team's queue and we'll try to get back to you within the next two weeks.

lesliecdubs avatar Dec 26 '24 23:12 lesliecdubs

Hi there @HDinger! Tried to see how to get everything passing CI wise and I think for this project we needed a PR from a maintainer for the snapshots to be committed back to the PR. As a result, I opened up: https://github.com/primer/view_components/pull/3253 and cherry picked the commit you made. Hope that's okay! Let me know if not πŸ‘

joshblack avatar Jan 08 '25 19:01 joshblack

Closing this one out as we cherry picked the commit over into: https://github.com/primer/view_components/pull/3253 so that the snapshots would generate. Thanks so much for contributing this! Let me know if you have any questions or concerns πŸ˜„

joshblack avatar Jan 09 '25 16:01 joshblack

Awesome, thanks for the effort! πŸ™‡ Glad to see it being part of the code πŸ‘ ☺️

HDinger avatar Jan 10 '25 07:01 HDinger