-
Notifications
You must be signed in to change notification settings - Fork 74
Add CTA to toasts #1263
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 CTA to toasts #1263
Conversation
| export const mockLocation = (mockedProperties) => new Proxy({}, { | ||
| get: (_, name) => | ||
| (Object.prototype.hasOwnProperty.call(mockedProperties, name) | ||
| ? mockedProperties[name] | ||
| : window.location[name]) | ||
| }); |
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.
For the tests of the CTAs, I want to mock window.location.reload(). window.location doesn't make that easy for us: sinon.replace() isn't enough. That's why I'm using this proxy. It allows us to mock specific properties like reload while returning actual values from window.location for other properties.
| }); | ||
| await wait(); | ||
| asyncRoute.should.alert('danger'); | ||
| asyncRoute.vm.$container.alert.ctaHandler(); |
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.
Here we call alert.ctaHandler() directly. We can't click .alert-cta because the alert isn't rendered in the DOM. This test just mounts the AsyncRoute component, not the whole app. We independently test alert.ctaHandler() and .alert-cta elsewhere, so I think it's OK to call alert.ctaHandler() directly here.
| .afterResponse(app => { | ||
| clock.tick(0); | ||
| .afterResponse(async (app) => { | ||
| await clock.tickAsync(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.
tick() needs to become tickAsync() in order to give the DOM a chance to update. We need .alert-cta to be rendered before we click it.
|
@ktuite, I'm thinking it might be easiest to review this one interactively. |
This PR makes progress on getodk/central#1011 and getodk/central#1030:
The CTA is not styled/positioned correctly yet. I'll do that in a follow-up PR as I continue working on #1011. I'm making this change now so that I can close out #1030 quickly. This is what the CTA looks like now:
What has been done to verify that this works as intended?
New tests. I also tried out one of the CTAs locally.
Why is this the best possible solution? Were any other approaches considered?
My goal was to make it as simple as possible as possible to add a CTA. I think that's going to be a relatively common pattern. For example, I don't want us to have to write a component/HTML whenever we want to add a CTA. I wanted something generic/reusable.
We also need something that will work outside of component code. For example, src/util/session.js needs to add a CTA.
I'll also add comments explaining individual lines.
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes