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

Modal Dialog Example: Increase z-index of dialog backdrop #2843

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

emamoah
Copy link
Contributor

@emamoah emamoah commented Oct 26, 2023

Relates to #2842.

Initially considered a bottom-padding adjustment, but that would create visual inconsistencies.
This solution seemed best, since, in my opinion, the user would not need to use the "Back to Top" link when the modal is open.

Preview link

Modal Dialog Example in compare branch

Review checklist

Reviewers: To learn what needs to be covered by each review, Follow the link for the type of review to which you are assigned.


WAI Preview Link (Last built on Thu, 26 Oct 2023 07:26:25 GMT).

Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

Looks good to me, fixes the problem.
Tested on my iPhone and the close buttons not obscured and the close button works.

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Issue 2842 - Bug in modal dialog example when accessed on mobile.

The full IRC log of that discussion <jugglinmike> Topic: Issue 2842 - Bug in modal dialog example when accessed on mobile
<jugglinmike> Jem: jongund reviewed; we need one additional reviewer
<jugglinmike> Matt_King: This is a CSS change. I don't feel comfortable reviewing it myself
<jugglinmike> The patch is gh-2843
<jugglinmike> github: https://github.com//pull/2843
<jugglinmike> Andrea_Cardona: I can take a look at this. I should be able to do that by the end of the day
<jugglinmike> Matt_King: Sweet!
<jugglinmike> Jem: I'll assign you on the pull request

@andreancardona
Copy link
Contributor

tested and looks good to me!

@mcking65
Copy link
Contributor

mcking65 commented Dec 5, 2023

@howard-e

Why were no checks run on this? At a minimum, shouldn't CSS Linting action have run? I don't see anything about it in the checks. The only thing there is the preview build.

@howard-e
Copy link
Contributor

howard-e commented Dec 11, 2023

@mcking65 I tested this change in my own fork, howard-e#60 and confirmed that the CSS Linting step does run. I also see other recently pushed PRs where the CSS Linting action works as expected, so this should be fine.

My only guess is that because this is Emmanuel's first PR in this repo, and there would have been an Approve to run workflows. Maybe a GitHub bug may have prevented that from running properly depending on when that button was clicked (but this is a huge guess). You can try pushing a[n] [empty] commit to this branch if we'd like.

Screenshot showing this is Emmanuel's first PR in a w3c repo

@mcking65 mcking65 merged commit d057791 into w3c:main Dec 11, 2023
1 check passed
@mcking65 mcking65 added the enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants