Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add new Modal Dialogue component #1668
Add new Modal Dialogue component #1668
Changes from all commits
7be5dcf
aae7ff1
956d5ae
e346314
aba92ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to use
!default
here so that the value can overridden.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increase to 1000 so that this will appear above all other elements already in the site.
I had to do this when I've used this code as several elements floated on top of the backdrop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to increase this, just in the (non-ideal) case that the page has other competing elements. I notice that the GOV.UK Publishing equivalent sets this to 1000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made this 1001 so it is above the backdrop which also needs the z-index increasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be okay as it is but just for reference, I notice that the GOV.UK Publishing example currently sets these to:
I don't think we currently have an example with page content in the main area so we need to add one (see #1668 (comment)) in order to check that the main page content is not scrollable and the modal is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to safely remove this font definition as I wanted the site's standard fonts to be used. We're a non gov body so can't use the standard GDS font and have overrides in place to not use them. This line being included broke our GDS font overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this block should live below https://github.com/alphagov/govuk-frontend/pull/1668/files#diff-40be96fa6566e06e9d9d9ab4d1d3d831R8 to in order to keep the backdrop styles together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the mobile/tablet experience meant to be similar to the GOV.UK Publishing modal?
This might be something for @Ash-Wilson to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these styles are set separately from the other ones in this file. Should this be a modifier class?