Conversation
meganrm
requested review from
frasercl and
toloudis
and removed request for
a team
| } | ||
|
|
||
| export enum Section { | ||
| LandingPage = 0, |
There was a problem hiding this comment.
did you reserve 0 on purpose, knowing you were eventually going to add this?
There was a problem hiding this comment.
yeah, it was also easier for having the page number system be 1 indexed.
src/content/HighAffinity.tsx
Outdated
| doi:10.2210/rcsb_pdb/goodsell-gallery-021 | ||
| </div> | ||
| <div> | ||
| This painting is part of “VAX,” a series of paintings |
There was a problem hiding this comment.
I think when it's a title you can put the comma after the closed quote..
There was a problem hiding this comment.
oh you're right, I copied this text from his website
frasercl
left a comment
frasercl
left a comment
There was a problem hiding this comment.
One larger organization comment that I think is likely worth addressing, but looks good overall
| section, | ||
| landingPage, | ||
| }) => { | ||
| const getContent = () => { | ||
| if (section === Section.LandingPage) { | ||
| return landingPage; | ||
| } else { | ||
| return content; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Currently, both the landing page and content are rendered before they are passed to this component, but only one is ever on screen. Could this content picking function be lifted to App instead, where it can select which JSX to actually render? Might make more sense there anyways, since beyond a class name there's not really a layout difference between landing page and content. Alternatively, the types of landingPage and content could become () => ReactNode?
| > | ||
| <div className={styles.content}> | ||
| <h1>{title}</h1> | ||
| {content} |
There was a problem hiding this comment.
Bit surprised this isn't children... but I see this prop comes from an interface that's used in a number of other components
| Welcome to the Binding Affinity interactive application! | ||
| Here, you will learn how to determine the strength of | ||
| interaction between two molecules in the lab by using | ||
| computer simulations.{" "} |
There was a problem hiding this comment.
explicit space doesn't appear to be necessary here?
| computer simulations.{" "} | |
| computer simulations. |
| export const FIRST_PAGE = { | ||
| [Module.A_B_AB]: 0, // landing page | ||
| [Module.A_C_AC]: 1, | ||
| [Module.A_B_C_AB_AC]: 1, | ||
| }; |
There was a problem hiding this comment.
could/should this be expressed in terms of the Section type? (e.g. [Module.A_B_AB]: Section.LandingPage,?) Or does that capture a different concept?
3 participants
Problem
Estimated review size: small
closes #190
Solution
Added a landing page that renders at the beginning of the app
Type of change
Please delete options that are not relevant.
Steps to Verify:
Screenshots (optional):