-
Notifications
You must be signed in to change notification settings - Fork 2
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
[feat] finalize onboarding styling #64
[feat] finalize onboarding styling #64
Conversation
b5a63e7
to
4a9d70e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking amazing so far! I rebased + added some styling changes to OnboardingContainer, since I was assuming that we'd move OnboardingContainer out of the individual questions and into the OnboardingFlow component.
However, that resulted in the Review page being messed up. You can revert to your original style for OnboardingContainer if you'd like, but i think it's worth considering ways to simplify + reudce redundancy.
Also, after rebasing, there were some issues with the style of the Progress bar against the Header, which we should address
app/onboarding/page.tsx
Outdated
<ProgressBar progress={2.6} /> | ||
<P3 $color={COLORS.shrub} style={{ marginTop: '2.5rem' }}> | ||
QUESTION 1 OF 3 | ||
</P3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the progress bar and the question number are shared between multiple pages, we should move it outside of these individual components and into the OnboardingFlow component to reduce redundancy
If we end up making onboarding a multi-page flow next sem, these would be good candidates of things to include in the layout.tsx
So the individual question components can primarily just be the Question Title + the RadioOptions
we might even consider creating a general component OnboardingQuestion
that can be used for all 3 questions
app/onboarding/page.tsx
Outdated
<ButtonDiv> | ||
<Button | ||
onClick={onNext} | ||
disabled={!selectedState} | ||
$primaryColor={COLORS.shrub} | ||
> | ||
Next | ||
</Button> | ||
</ButtonDiv> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it'd make sense to move the next/back buttons to OnboardingFlow as well
c8b45a4
to
accda5c
Compare
app/onboarding/page.tsx
Outdated
</BigButton> | ||
</ReviewContainer> | ||
</PageContainer> | ||
<LabeledCustomSelect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: instead of creating a new component, we want to just add a new prop to CustomSelect
app/onboarding/page.tsx
Outdated
<BigButton color={COLORS.shrub} onClick={handleSubmit}> | ||
Let's Start Growing! | ||
</BigButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be both a "Back" and a "Let's Grow!" (Submit) button here.
We can copy over the button styles from the other screens here.
accda5c
to
057b35d
Compare
min-height: 100vh; | ||
background-color: ${COLORS.seed}; | ||
export const OnboardingContainer = styled.div` | ||
min-height: calc(100vh - 60px); // 60px is the hardcoded height of Header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably change this to use Flex full-screen
so that there's a single source of truth in terms of the screen height
components/ProgressBar/styles.ts
Outdated
|
||
export const BackgroundDiv = styled.div` | ||
width: 100%; | ||
height: 0.375rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be px
onClick={isContainerClickable ? () => setIsOpen(!isOpen) : undefined} | ||
> | ||
<P2 $color={COLORS.midgray}> | ||
{options.find(option => option.value === value)?.label || placeholder} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to refactor DropdownOption<T>
to Record<T, string>
, i.e. use the values as keys, to mitigate needing to search through options
for the corresponding label.
<OptionsContainer> | ||
{options.map(option => ( | ||
<Option | ||
key={String(option.value)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the key can just be an index since the options list is static, but maybe if we foresee ever passing in dynamic options, this might be a good idea
plantsToAdd: Plant[]; | ||
setProfile: (completeProfile: Profile) => Promise<void>; // Now expects full Profile | ||
loadProfile: () => Promise<void>; | ||
setHasPlot: (plotValue: boolean | null) => void; | ||
setPlantsToAdd: (plants: Plant[]) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should prob separate setPlantsToAdd
and plantsToAdd
out into a separate context. otherwise, updating setPlantsToAdd would trigger a re-render of the entire app
What's new in this PR 🧑🌾
Description
Screenshots
Onboarding:
Add Details
View Plants (see Clear Filters Button)
How to review
Next steps
Relevant links
Online sources
https://www.developerway.com/posts/react-re-renders-guide: on how to mitigate re-rendering
Related PRs
CC: @ccatherinetan