Courses: update UI for course catalog & hoc options #16316
Conversation
|
cc @tanyaparker |
This reverts commit 99c0e0e.
|
Erin is out so can you assign another dev to review? Is the Local Classes box localized? Ideally I want this to only show for US users (whether they are in English or non-EN). It's ok if it shows up for non-EN now as long as it's US only AND it's localized. Otherwise, if we can't target by country OR if it's not localized, I would rather hide it completely for all non-EN. Also, is the Hour of Code widget localized? I don't want to switch it out just yet, but in some time (after we get some translations) we can replace the 8 block version with the 4 block version so we have fewer versions/more consistency. |
| @@ -56,7 +56,7 @@ | |||
| - unless course[:starts] == 0 | |||
| %div{class: "bar-#{course[:starts]}"} | |||
| | |||
| %div{class: "courserow bar-#{course[:ends] - course[:starts] + 1}", style: "background-color: #{"#1AC7D6" || course[:color] || '#00adbc'}"} | |||
| %div{class: "courserow bar-#{course[:ends] - course[:starts] + 1}", style: "background-color: #{"#00adbc"}"} | |||
There was a problem hiding this comment.
I like sticking to our color palette a lot better. 👍
There was a problem hiding this comment.
presumably we don't need the #{} portion now that there's no ruby in there (just a string)
| @@ -492,13 +492,15 @@ | |||
| "teacherCommunity": "Teacher Community", | |||
| "teacherCommunityDescription": "Ask questions about curriculum, share ideas from your lessons, and get help from other teachers", | |||
| "teacherCourseHoc": "Hour of Code", | |||
| "teacherCourseHocDescription": "Celebrated in December, but available year-round, the Hour of Code makes computer science fun and accessible to all ages.", | |||
| "teacherCourseHocDescription": "If you don’t have time for a full length course, try a one-hour tutorial designed for all ages. Join millions of students and teachers in over 180 countries by starting with an Hour of Code.", | |||
There was a problem hiding this comment.
Can we link Hour of Code?
There was a problem hiding this comment.
@Bjvanminnen I tried once and failed.. do we have a good way to embed links in apps strings, wrapping the link around some text that's in the string?
There was a problem hiding this comment.
We don't have a good way to do it inside of translated strings. The approach I usually aim for is to have a link following the string.
There was a problem hiding this comment.
@tanyaparker Can we rewrite the content to make that work?
There was a problem hiding this comment.
Maybe append "Visit HourOfCode.com to learn more." at the end where the whole thing is hyperlinked? Is that what you mean?
| /> | ||
| </ContentContainer> | ||
| linkText={i18n.teacherCourseHocLinkText()} | ||
| link={`${codeOrgUrlPrefix}/hourofcode/overview`} |
There was a problem hiding this comment.
I'm not sure I'm reading this right but wanted to confirm teachers go to code.org/hourofcode/overview but students go to code.org/learn?
There was a problem hiding this comment.
Signed-in student in English gets /learn.
Signed-in teacher in English gets /hourofcode/overview.
| {/* Anything but a teacher or student in English. | ||
| That is: signed-out, or a student or teacher in non-English. */} | ||
| {(isSignedOut || !isEnglish) && ( | ||
| <div id="all-courses"> |
There was a problem hiding this comment.
Does this div need an id? I don't see it used anywhere (we do look for a class by this name in componentDidMount)
There was a problem hiding this comment.
Oh, I just added this to make my own visual inspection of the DOM a bit easier. I can take it back out again if not a good practice.
There was a problem hiding this comment.
When I'm reading JSX and I see an id, my first thought is "that means there are side effects somewhere else that I need to track down" (either CSS, or jquery doing something). To some extent it's conceptually similar to adding a global variable (although arguably not quite egregious). As such, I prefer avoiding them unless absolutely necessary.
|
Local Classes box is localised. I'm waiting to hear back from infrastructure folks about perf impact of calling Geocoder on every page, so in the meantime I'll just make it EN only for now. The Hour of Code content is also localised. The boxes themselves are the legacy HAML. |
signed-in teacher, english
signed-in student, english
signed out, english
signed-in teacher, non-english
signed-in student, non-english
signed out, non-english