Skip to content

Add upcoming events to landing page#398

Open
ayush-goyal wants to merge 13 commits into
developfrom
ayush/357-new-upcoming-events-carousel-landing
Open

Add upcoming events to landing page#398
ayush-goyal wants to merge 13 commits into
developfrom
ayush/357-new-upcoming-events-carousel-landing

Conversation

@ayush-goyal

Copy link
Copy Markdown
Contributor

Closes #357

@github-actions

github-actions Bot commented Nov 15, 2020

Copy link
Copy Markdown

Deploy preview for gen-soln ready!

✅ Preview
https://gen-soln-cynn3irle.vercel.app

Built with commit f3a64c0.
This pull request is being automatically deployed with vercel-action

@rithikgavvala

Copy link
Copy Markdown
Collaborator

There are a couple concerns with this PR. One being the placement of the carousel should be below the discover section not below new non-profits on the platform. Also the layout of the cards is wonky.

@PC98

PC98 commented Nov 16, 2020

Copy link
Copy Markdown
Collaborator

The position of the "Upcoming Volunteer Events" text and the position of the first card in the list seems to vary depending on number of cards being displayed, but this should not happen.
Screen Shot 2020-11-15 at 6 39 55 PM

I would recommend looking at what styles the /events page uses and emulating them.

{/*
<img
src={footerImage1}
src="../backgrounds/support_us.png"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try just "/backgrounds/support_us.png" instead of relative path

marginTop: "140px"
}}
onClick={() => {
void router.push(config.pages.donate("64357724"));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of hardcoding this, I am okay if this button doesn't have an on-click action

nonprofitCardData={cardData}
onClick={() => {
window.location.replace(config.pages.nonprofit(cardData._id));
window.location.replace(config.pages.donate(cardData._id));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change correct?

upcomingEventsCarousel: {
display: "flex",
flexDirection: "column",
"align-self": "center",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use camel case, alignSelf, same for other properties

Comment on lines +106 to +109
<div className={allLink}>
<CoreLink href={"/events"}>ALL EVENTS HERE</CoreLink>
<LongArrowRight className={arrow} />
</div>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using the CoreButtonWithLongArrow component instead

Comment on lines +118 to +121
<div className={allLink}>
<CoreLink href={"/"}>ALL NON-PROFITS HERE</CoreLink>
<LongArrowRight className={arrow} />
</div>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for here, use the component from core/buttons/

Comment thread pages/index.tsx
page: number;
totalCount: number;
};
| PaginatedNonprofitCards

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you check if just nonprofitCards: PaginatedNonprofitCards will suffice?

const { page, isLastPage, cards } = nonprofitCards;
const {
container,
text,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please delete these unused styles from makeStyles

}

const Home: React.FC<Props> = props => {
const [session] = useSession();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete?

import { NonprofitCardData, PaginatedNonprofitCards } from "utils/types";

import NonprofitCard from ".././cards/NonprofitCard";
import NonprofitCard from "../cards/NonprofitCard";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer using module aliases, import { NonprofitCard } from "@core/cards";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add upcoming events carousel to the landing page

6 participants