-
Notifications
You must be signed in to change notification settings - Fork 10
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
USAGOV-1899 Benefits Redirect Modal #1896
Conversation
It would be nice if we could avoid the need to use this div to associate the modal-name with its numerical ID assigned by Drupal's paragraph system. usagov-2021/web/themes/custom/usagov/templates/region--header-top.html.twig Lines 37 to 39 in 4ef89d0
As far as I can tell, this would require us to call our modal template directly rather than letting Drupal render the content block entity with Drupal's internally assigned IDs (which currently happens on line 37 above). usagov-2021/web/modules/custom/usa_twig_vars/usa_twig_vars.module Lines 337 to 340 in 4ef89d0
|
Hello! I'm still reviewing this but since I noticed that it shares a couple of things with the site banner, I wanted to let you know that I changed the way site_banners are added completely because of the cache issues. If this works we should be fine but if you're curious here is the PR: #1769 |
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 put a couple of questions in here, and they are just questions. I haven't had as much time as I'd like to look at this!
It worked well for me and I wasn't able to reproduce the problem I described with the original site banner implementation, nor am I seeing the problem where Drupal config sync warns that importing the config will delete the blocks I created. We (three) should probably look into why I'm seeing that issue on her PR and not on this one.
I'm ready to approve this to get it on dev, while we probably do want to follow up and maybe make some additional changes.
…nd add comments for clarity.
…ts-Redirect-Modal' of https://github.com/usagov/usagov-2021 into USAGOV-1899-Benefits-Redirect-Modal
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 is ready for review on dev. There are two things I know of that should be in a follow-on:
- Sanitize that modal name after pulling it from the query parameters. I'm thinking that rather than use something like DomPurify, we should just restrict modal names to (letters, numbers, dashes, and underscores).
- Put that same restriction in the form for creating/editing the block
- Default "new revision" to TRUE when saving a block. Actually, don't let people turn that off.
modal.hidden = false; | ||
}); | ||
|
||
let modalName = removeUrlParameter('modal'); |
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.
Let's sanitize this modalName here. (I think here is better than within "removeUrlParameter." )
Jira Task
https://cm-jira.usa.gov/browse/USAGOV-1899
Description
This adds a modal block-type for CMS users to create modals for the website.
It includes javascript to check the url for a modal query parameter and display a modal if one with a matching name exists.
Type of Changes
Testing Instructions
/admin/content/block
+ Add content block
Modal
Modal ID
will be used in the URL query parameter so keep it URL safeButton Text
can be anything since this button remains hiddenYes Button Text
will be displayed as the button to exit the modalNo Button Text
should be left blankSave
modal_id
" to end of the URLChange Requirements
Validation Steps
Security Review
Reviewer Reminders
Post PR Approval Instructions
Follow these steps as soon as you merge the new changes.
Review in Test
and add a comment. State whether the change is already visible on cms-dev.usa.gov and beta-dev.usa.gov, or if the deployment is still in process.