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.

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

782 give every modal a header with title and close button #829

Conversation

dhinrichs-scottlogic
Copy link
Contributor

@dhinrichs-scottlogic dhinrichs-scottlogic commented Feb 7, 2024

Description

Every Modal now has a heading and a close button with the text "close" and an x icon.

Screenshots

image
image
image

Notes

  • new OverlayHeader component
  • new x icon svg
  • style update for DocViewer and Handbook

Concerns

  • I took the svg from here and then removed anything I didn't understand that seemed unnecessary, so would be great to double check that I didn't remove anything important.

Checklist

Have you done the following?

  • Linked the relevant Issue
  • Added tests
  • Ensured the workflow steps are passing

@dhinrichs-scottlogic dhinrichs-scottlogic linked an issue Feb 7, 2024 that may be closed by this pull request
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

I've not done a proper review yet, but I noticed that on the handbook the header is overlapping the top of the scroll bar:

image

Then I realised how the handbook header was structured:

  • The overlay-header div sits at the top with position:fixed while the handbook-overlay div sits tucked underneath, and it's up to some clever margins to nudge the content and spine down a bit below the header.

image

image

And while this looks grand if you get the margins just so, It seems vulnerable to visual bugs down the line.

Ideally we'd have the overlay-header actually take up space in the div, and have the spine and content fill up the remaining space. I had a go at that in this separate branch (You can click compare and pull request to see the changes). It would require some more css fiddling to get the other modals looking good again though, since they all depend on the overlay-header having position:fixed.

@pmarsh-scottlogic
Copy link
Contributor

Visual changes look cracking by the way! Should've led with that 😅

Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

Excellent work, looks amazing ❤️

Just a handful of minor suggestions.

@dhinrichs-scottlogic
Copy link
Contributor Author

@pmarsh-scottlogic Chris did some css magic, the issue with the scrollbar and margins should be fixed and future-proofed now.
image

Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

Lovely ❤️

@dhinrichs-scottlogic dhinrichs-scottlogic merged commit 9ad9a5e into dev Feb 8, 2024
2 checks passed
@dhinrichs-scottlogic dhinrichs-scottlogic deleted the 782-give-every-modal-a-header-with-title-and-close-button branch February 8, 2024 16:32
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.

Give every modal a header with title and close button
3 participants