-
Notifications
You must be signed in to change notification settings - Fork 481
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
AFE Call to Action Part 1 #35340
AFE Call to Action Part 1 #35340
Conversation
@@ -226,11 +222,7 @@ export default class TeacherHomepage extends Component { | |||
)} | |||
{isEnglish && this.state.donorBannerName && ( |
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.
Since this component is already here, assuming this PR won't actually make it visible for anyone yet? Does this.state.donorBannerName
control who sees the banner?
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.
yep donorBannerName will be null if the banner should not be shown
I can't wrap my head around how |
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.
LGTM -- would maybe think about getting rid of donorTeacherBanner.js
entirely if it's not used anywhere? Also may be worth having someone in @code-dot-org/plc look at least at the amazon_future_engineer.js
file, asynchronous JS is not quite my jam yet :).
removed the donorTeacherBanner since it's now not used anymore |
Is donorTeacherBanner.js not used in Phase 2 of the campaign (which is roughly similar in user flow to last year's campaign, where a donor teacher banner was shown on the dashboard, for select schools)? |
@dju90 I think the name of this PR is maybe misleading you a bit, this PR represents "Phase 2" of the spec, but is a "Part 1" of that phase :). It's not using |
That's correct! We aren't using donorTeacherBanner anywhere but the homepage in phase 2 which means we don't need that wrapper anymore, which was used to embed the banner within markdown |
Update AFE Call to Action based on spec for this year (this is Phase 2 in the spec). Changes made are:
The banner now looks like this:
![Screen Shot 2020-06-16 at 9 06 41 AM](https://user-images.githubusercontent.com/33666587/84802146-08373a80-afb5-11ea-8497-2169d5ad226c.png)
The new form is embedded at the bottom of /amazon-future-engineer (see Next Steps for details on this)
![Screen Shot 2020-06-16 at 9 06 09 AM](https://user-images.githubusercontent.com/33666587/84802156-0b322b00-afb5-11ea-9c1f-1fd7e42ce0e1.png)
Next Steps
For now I have just put the new form as-is on the /amazon-future-engineer page, which means it starts with "Am I Eligible?". We want to instead skip over this step. The ability to do that will be added to the new form soon so skipping over that step on /amazon-future-engineer will be done as a follow-up item.
Links
Testing story
Tested manually on different browsers for teachers who are and are not eligible for AFE.
Reviewer Checklist: