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.

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

Update HOC tiles on /courses and /sign_in #25570

Merged
merged 16 commits into from Nov 9, 2018
Merged

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Oct 23, 2018

Updates the course tiles on /courses and /users/sign_in.

Some examples of those with the new Dance Party tile:

On /users/sign_in:
screen shot 2018-11-08 at 3 02 54 pm

On /courses:
screen shot 2018-11-08 at 3 02 36 pm

For both of these pages, users with different languages settings should see the following course tiles when the DCDO.hoc_launch flag is set to different values:

-- "mc" "dance" nil
English users minecraft aquatic, starwars, frozen, classic maze dance party, minecraft aquatic, app lab, flappy code minecraft hero, starwars, frozen, classic maze
non-English users [same as English users] dance party, minecraft aquatic, frozen, classic maze [same as English users]

@maddiedierker maddiedierker changed the base branch from staging to mc-aquatic-hoc October 23, 2018 22:53
@maddiedierker maddiedierker changed the title Update HOC tiles on /courses and /sign_in [WIP, do not merge] Update HOC tiles on /courses and /sign_in Oct 24, 2018
@maddiedierker
Copy link
Contributor Author

@maddiedierker maddiedierker changed the base branch from mc-aquatic-hoc to staging October 31, 2018 00:07
@maddiedierker maddiedierker changed the title [WIP, do not merge] Update HOC tiles on /courses and /sign_in Update HOC tiles on /courses and /sign_in Nov 8, 2018
@maddiedierker
Copy link
Contributor Author

@poorvasingal can you take a look at the table i wrote out in the description of this PR to make sure it's correct? i laid out which course tiles English/non-English users should see based on which DCDO.hoc_launch value is set and want to make sure i didn't get any mixed up. thank you!

@Erin007
Copy link
Contributor

Erin007 commented Nov 9, 2018

The Minecraft text is a little squished with "problem" on the right hand side. Does it need a little padding?
screen shot 2018-11-08 at 7 35 39 pm

Also intentional that the /courses boxes have the green "Try now" band on the bottom, but the sign in ones don't. Just curious about the consistency.

// First row, dynamically created based on hocLaunch value
const tiles = this.getFirstRowTiles();
tiles.forEach((tile, index) => {
$(tile).appendTo(ReactDOM.findDOMNode(this.refs[index]));
Copy link
Contributor

Choose a reason for hiding this comment

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

snazzy. I like how you consolidated these!

</div>

{this.props.rowCount > 1 && (
<div>
<br/>
<br/>
<div className="row">
{/* TODO: (madelynkasula) If Flappy is in 1st row, what should replace Flappy on line below? */}
{/* Can we remove 2nd row? It's never used... */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any resolution to this yet? I'm curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, i totally forgot--thank you for reminding me! 😂 i'll talk to poorva and address in a follow-up

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 few comments, but looks great so far!

@poorvasingal
Copy link
Contributor

Hey Maddie - Double checked the comments from Marina on the spec and your table looks right based on that. The main thing I'm unsure about is if the DCDO flag is going to be changed back to "nil" after HOC? B/c we'd want the tiles you have for the dance launch to continue to stay post-HOC.

+1 to ErinB's comment on padding for the Minecraft tile. Seems like we need additional margin on the right in the tiles. I think this is true of each tile, just that it's only noticeable on MC in English b/c of the way the text ends up wrapping.

ErinB - The "Try now" inconsistency is as expected. We only show the "Try now" band when you're signed in (even on the courses page). It's because it turns into "Continue" if you've already made progress in the tutorial, which we only know for a user if they are signed in.

@maddiedierker maddiedierker merged commit fa96c81 into staging Nov 9, 2018
@maddiedierker maddiedierker deleted the hoc-dance-course-tiles branch November 9, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants