Missing space between Dialog Header and Body
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 | ||
| With large header and divider | ||
| Custom header | ||
| Header filter |
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.
π¦ 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
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.
Hi @team can I please get any feedback on this?
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?
Hi everyone :) Are there any updates on this? Is there anything I can do to help? /cc @lukasoppermann @camertron @lesliecdubs
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.
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 π
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 π
Awesome, thanks for the effort! π Glad to see it being part of the code π βΊοΈ