-
Notifications
You must be signed in to change notification settings - Fork 7
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 Modal component and integrate Mailchimp form #96
Conversation
margin-left: 20px; | ||
margin-right: 20px; |
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.
Possible shorthand can be used?
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 didn't want to override the top and bottom margin settings from the non-media-query version of this, which are set to 80px and 40px respectively.
bottom: 0; | ||
left: 0; | ||
overflow: auto; | ||
position: fixed; | ||
right: 0; | ||
top: 0; |
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 assume you tried other methods in completely covering the whole view of the browser with the darkened hue of the modal but is there a shorter way to cover the whole screen than putting all bottom, top, left and right positions to 0?
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 actually didn't try anything else because I had copied these styles over from the default ones that the library comes with. I'm not sure if there's a shorter way to cover it, since it's position: fixed
. You still need the top: 0
since the element itself is placed based on where its parent is, not based on the page. The right
and the bottom
could be replaced with width: 100vw
and height: 100vh
, which doesn't save us much because it's a one-for-one tradeoff.
You could possibly leave out left:
, but it will only work if you don't have anything that pushes the modal's parent's position away from the left edge, which makes it a little bit riskier to do.
Overall, I think setting the position across all four dimensions is pretty much as compact and reliable as we're going to get.
0543d44
to
2518cf9
Compare
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.
Great fix!
2518cf9
to
3a34ba0
Compare
Overview
This got a bit more complicated and messier than I had wanted, but I think this is in good enough shape to start reviewing.
This really does a few things at once, all related to the Mailchimp form modals on the site:
react-modal
library, and it sets up a standalone, reusable Modal component that can be used for other things in our site.Modal Component
f9f6f6e
I added a standalone Modal component to
src/components/grid-aware/Modal
based on the styles found in our Figma designs.This component just sets up the "outer" parts of the modal, and it expects the insides to be populated by the caller via the children. Specifically, this component sets up the overlay, which is the gray, partially transparent background, the modal container itself, and the close button. It's stateless, so it expects that its state is managed by its parent, passing in the
isOpen
prop as well as thesetIsOpen
function prop.There's some weird stuff going on with this
ReactModal.setAppElement()
thing, which is something that the library uses for accessibility purposes, where it wants to know the top-level element containing the application, but not containing the modal itself, so that can indicate to screen readers that the app element should be "disabled" when the modal is open. For us, unfortunately, this causes a chicken-and-the-egg problem, because on the very first render, our modal component is actually rendered before the app element, meaning that if you try setting the app element before the app element is actually instantiated on the page, react-modal will hit an error. In Storybook, I worked around this by disabling the accessibility feature, but in our actual app, I had to do something weird where I useduseEffect()
to only set the app element after the first render.I haven't fully researched all of the properties from react-modal, and it's possible that we may want to set a few more of those, so I can look into that next.
Mailchimp form
5d63d66
This does a direct embedding of the Mailchimp forms using the snippet that we get directly from Mailchimp. Initially, I thought that this would be more maintainable, since it allows the non-technical teams to just send the technical team a direct copy of the snippets from Mailchimp, but this got a bit messier than I expected with overriding styles, and I also realized that the snippets just directly contain the input fields, rather than totally generating them at runtime with JavaScript.
Let me at least explain what's going on in the current approach, regardless of what we choose to do with this in the future. The first is that I am using React's
dangerouslySetInnerHTML
prop, which allows you to embed any raw HTML as a string, including in-document<style>
or<script>
tags. Next, I have two different CSS files: a CSS modules file and then a non-module, global CSS file. The former is for the title and the description of the form, which do not come with the Mailchimp embed code, and the latter is for overriding the styles from Mailchimp.I made liberal use of
!important
, since Mailchimp has some pretty high-specificity styles, although I probably could have increased my own specificity by adding an extra global CSS class to wrap the whole thing. I had to duplicate our site styles for the buttons and input elements, since I had to make them apply to and override Mailchimp's styles.Adding the modal to the Home page
5523480
This was mostly straightforward, but I did have to ident almost every line in the Home page source code, since I needed to set up some React state with
useState()
to control the modalisOpen
prop. For me, rebasing this branch on top ofmain
has been a bit of a pain, since I'm constantly getting merge conflicts, but I don't think there's much I can do here until this is merged in.Things that still need to be addressed
@derekfidler, I'm guessing this is not a great UX, so maybe we really should just replace the embedded form with our own, so that we can handle the form submission entirely in JavaScript.
There's some weird re-rendering and pop-in that happens when the modal opens on the actual Home page. I'm guessing that for some reason, a re-render is occurring for all the elements on the base Home page, and because our images are getting reloaded, it causes the page to re-layout with the images popping in and out, shifting elements on the page.
The Mailchimp form styles are completely not loading in Firefox for some reason.
I am apparently failing some tests, which I think is related to TypeError: parentInstance.children.indexOf is not a function reactjs/react-modal#553.