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.

Sign up for GitHub

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

Jump to bottom

Recruitment: restart banners #34762

Merged
merged 5 commits into from May 12, 2020
Merged

Recruitment: restart banners #34762

merged 5 commits into from May 12, 2020

Conversation

breville
Copy link
Member

@breville breville commented May 12, 2020 •
edited

@breville breville requested a review from a team as a code owner May 12, 2020 06:51
@Erin007
Copy link
Contributor

Erin007 commented May 12, 2020 •
edited

The call to action everywhere is "Join us", except on /yourschool where it's "Apply Now" - double checking that's intentional?

@Erin007
Copy link
Contributor

Erin007 commented May 12, 2020

I have nit picky wonders about the signed out homepage buttons and text update because it's kind of unclear what I should be checking out. The text says "Check out our other home learning resources" but the "Check it out" button refers to Code Break. Maybe "Take a Break" instead of "Check it out"? 🤷

Erin007
Erin007 reviewed May 12, 2020
View reviewed changes
pegasus/sites.v3/code.org/views/homepage_hero.haml
- elsif entry[:type] == "code_break_video"
%div
%a{onclick: "return showVideo_mainvideo();", style: "cursor: pointer"}
%img{src: "images/homepage/watch_video_drawn_button.png", style: "width: 250px; margin-top: 10px; "}
%img{src: "images/homepage/watch_video_drawn_button.png", style: "width: 250px; margin-top: 10px"}
Copy link
Contributor

Choose a reason for hiding this comment

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

keeping this around in case we need it again even though it's not currently in use?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, yes. Also I forgot :)

@Erin007
Copy link
Contributor

Erin007 commented May 12, 2020

Looks like test failures are legit, but should be straightforward to resolve.

Erin007
Erin007 approved these changes May 12, 2020
View reviewed changes
Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

A couple questions, but code changes look good to me! Approved pending test updates.

@ericfershtman
Copy link
Contributor

Handful of comments:

  1. re Erin's comment about the Apply Now button on /yourschool: Let's change it to "Join us" so it's consistent with the other banners.

  2. re Erin's comment about buttons on Code Break version of the signed-out homepage: Good call, let's change it the top button from "Check it out" to "Take a Code Break" if that fits.

  3. The image for the callout box on the signed-in homepage and the view=teacher page is wrong (it's the nominate image). Can we change it to correct image?

@breville breville requested a review from islemaster May 12, 2020 16:53
@breville breville merged commit 9eaaaa9 into staging May 12, 2020
@breville breville deleted the restart-recruitment-banners branch May 12, 2020 18:02
Hamms
Hamms reviewed May 12, 2020
View reviewed changes
apps/src/templates/ProfessionalLearningApplyBanner.jsx
@@ -179,7 +179,7 @@ class ProfessionalLearningApplyBanner extends React.Component {
>
<div>
<button type="button" style={this.styles.button}>
Apply Now
Join us
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be translated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Professional Learning is only done in en...

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, looking at the code it looked like we were displaying these to everyone, regardless of language

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true we don't do a language check, but ProfessionalLearningApplyBanner is used by YourSchool which, while partially translated (to my surprise), is only intended for US schools.

pegasus/sites.v3/code.org/views/homepage_hero.haml
@@ -134,7 +142,7 @@
.right.col-50.mobile-center{style: "margin-top: 14px; padding-left: 5px; padding-right: 5px; text-align: center"}
%div
%img{src: "images/homepage/codebreak_logo_hand.png", style: "width: 400px; max-width: 100%"}
%h3{style: "font-weight: 800; color:#191919; font-size: 16px;"}Take a Code Break! Your weekly dose of inspiration, community, and computer science.
%h3{style: "font-weight: 800; color:#191919; font-size: 16px;"}Take a Code Break! Your weekly dose of inspiration, community, and computer science. And check out our other home learning resources!
Copy link
Contributor

Choose a reason for hiding this comment

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

also this

Copy link
Member Author

Choose a reason for hiding this comment

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

Code Break content is also only done in en...

breville added a commit that referenced this pull request May 12, 2020
@breville
Recruitment: update text for two banners
aee04e2 
Followup to #34762
@breville breville mentioned this pull request May 12, 2020
Merged
breville added a commit that referenced this pull request May 29, 2020
@breville
Recruitment: don't show banner on non-en teacher homepage
8e350cc 
Followup to #34762.  Don't show the recruitment banner on non-en teacher homepage.
@breville breville mentioned this pull request May 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hamms Hamms Hamms left review comments

@islemaster islemaster islemaster approved these changes

@Erin007 Erin007 Erin007 approved these changes

@ericfershtman ericfershtman Awaiting requested review from ericfershtman

@markabarrett markabarrett Awaiting requested review from markabarrett

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@breville @Erin007 @ericfershtman @Hamms @islemaster