Skip to content

fix: Modal header's min-height is now applied when header has no button#655

Merged
gnapse merged 3 commits intomainfrom
ernesto/modal-header-height
May 16, 2022
Merged

fix: Modal header's min-height is now applied when header has no button#655
gnapse merged 3 commits intomainfrom
ernesto/modal-header-height

Conversation

@gnapse
Copy link
Copy Markdown
Contributor

@gnapse gnapse commented May 13, 2022

Short description

This PR makes sure that the modal header retains the same minimum height that it would have if it featured a close button on the right. For modals with a button visible, the header has the correct padding around the title because the button height ensures so.

I believe this may have been introduced in #633, hence my request for Pedro Alves to review (to make sure this fixes the issue without re-introducing the issue that #633 was attempting to fix).

BeforeAfter
CleanShot 2022-05-13 at 16 37 10@2x CleanShot 2022-05-13 at 16 34 59@2x

PR Checklist

  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all) (will do so after this is approved)

Versioning

New patch release.

@gnapse gnapse requested a review from pedroalves0 May 13, 2022 20:45
@gnapse gnapse self-assigned this May 13, 2022
Copy link
Copy Markdown
Member

@pedroalves0 pedroalves0 left a comment

Choose a reason for hiding this comment

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

It would be nicer if the parent elements set the minimum height instead of having a dummy div. I tried to implement that quickly but hit some alignment issues. I understand that could require some layout changes, so I'm approving as is.

@gnapse
Copy link
Copy Markdown
Contributor Author

gnapse commented May 16, 2022

It would be nicer if the parent elements set the minimum height instead of having a dummy div

I tried, but the parent element has a padding, hence I cannot set its height to 32px because that's not the final height that it ends up with. Its final height is 32px plus its top and bottom padding. I could compute that and use it, but then we'd need to update it if we ever change that padding. Hence, the most convenient approach is to make sure its content has 32px min-height, which will ensure the same overall height as when it has a close button.

@gnapse gnapse merged commit 2632e67 into main May 16, 2022
@gnapse gnapse deleted the ernesto/modal-header-height branch May 16, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants