-
Notifications
You must be signed in to change notification settings - Fork 3
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
Redesign tree page and add sections for site images and tree benefits #264
Conversation
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.
💯 redesign looks awesome!! Thanks for working on this, I really should have split this over multiple tickets
<StyledCarousel | ||
arrows | ||
prevArrow={<LeftOutlined />} | ||
nextArrow={<RightOutlined />} | ||
adaptiveHeight | ||
afterChange={onAfterChange} | ||
> |
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.
⛏️ For some reason, the onAfterChange
function wasn't being called when I changed images in the carousel, which meant the uploader's name and date weren't properly changing. Using the beforeChange
prop on the carousel works though, and I was able to get it working with this function:
const onBeforeChange = (_: number, next: number) => {
setCurrSlideIndex(next);
};
No idea why this prop works but afterChange
doesn't 😞
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.
Hmm is this what you're expecting to see if there are two images where one is uploaded by chromium52 and the other is by anonymous? I'm not 100% sure why but I can't seem to reproduce it on my end 🤔
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.
Yeah, that's what should be happening - might be an issue on my end somewhere else then 🤔. Let's just merge this and see this issue happens in prod then
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.
Ok sounds good!
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.
Just verified this looks good on prod
Checklist
npm run check
npm run test
npm build --if-present
Why
Resolves #240
Redesigns the tree page according to the Figma designs
Adds two new sections on the tree page for the site image carousel and tree benefits
Note: This PR doesn't include adding/deleting site images from the frontend
This PR
The most important changes are the two new components,
<SiteImageCarousel />
and<TreeBenefits />
, as well as restructuring the tree page components (specifically, putting them insrc/components/treePage/
) with a barrel file so that they're all in one place and not scattered throughout thecomponents
folderScreenshots
Verification Steps
Visually checked the tree page for several trees (http://localhost:3000/tree/3303432, http://localhost:3000/tree/3175896, etc.)