Generic parent letter (story) #34016
Conversation
apps/src/lib/ui/ParentLetter.jsx
Outdated
|
|
||
| const Header = () => { | ||
| return ( | ||
| <header style={{backgroundColor: color.teal, marginBottom: 30}}> |
There was a problem hiding this comment.
nit - our styleguide prefers extracting these to a style object rather than doing it inline.
There was a problem hiding this comment.
👍 Will update.
Independent of this PR, would love to have a conversation at some point about whether people actually like this rule: It's one we introduced without much discussion and I mostly find it annoying now. It makes composing styles a little unintuitive. I'm also not sure it's enforceable by any existing linter rule, and in general we try to cut things from our styleguide that we can't automatically enforce. What are your thoughts on it?
| ). | ||
| </p> | ||
| <h1>Step 2 - Get your child set up to use Code.org at home</h1> | ||
| <SignInInstructions loginType={loginType} sectionCode={sectionCode} /> |
There was a problem hiding this comment.
I like the extraction here - makes this much more readable!
apps/src/lib/ui/ParentLetter.jsx
Outdated
| </li> | ||
| <li> | ||
| If your student does not remember their password, they can reset it | ||
| from the Sign in screen |
There was a problem hiding this comment.
also nit - should 'In' be capitalized?
There was a problem hiding this comment.
Whoops! Pulled this as-is from the spec, but you're right, it's weird. I'd actually lower "sign." Our convention seems to be "Sign in" when quoting the button, and "sign in" everywhere else.
| email me and I will provide it | ||
| </li> |
There was a problem hiding this comment.
tiny nit: punctuation here? and line 181
There was a problem hiding this comment.
Which punctuation are you expecting? The convention in the spec was no punctuation at the end of any list items.
apps/src/lib/ui/ParentLetter.jsx
Outdated
| ); | ||
|
|
||
| const GoToSectionSignIn = ({sectionCode}) => { | ||
| const sectionUrl = `studio.code.org/sections/${sectionCode}`; |
There was a problem hiding this comment.
usually this and other urls would be relative using urlHelpers, but doesn't seem that important right now.
There was a problem hiding this comment.
🤦♂️ Oh, we should totally do that. Thanks for the reminder!
| storybook = storybook.storiesOf('ParentLetter', module); | ||
|
|
||
| // Make a story for every login type | ||
| Object.values(SectionLoginType).forEach(loginType => { |
| <ParentLetter | ||
| loginType={loginType} | ||
| sectionCode="ABCDEF" | ||
| teacherName="Minerva McGonagall" |
Introduces a generic parent letter component designed to be rendered by itself on a page, ready for printing or PDF generation; along with associated storybook entries. Implements most of Part 1, Requirement 5 of this spec.
I figured this was a good spot to stop, get a review, and merge in content that we can wire up to the dialog or PDF generation later.
The letter has variants for each login type:
Non-goals
We're intentionally not worrying about making this letter translatable yet. That follow-up work is tracked here.
Testing story
Developed entirely in storybook - nothing here will reach production yet.
Reviewer Checklist: