-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Learn More button to Nonprofit in Project #86
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks good! Left one comment about a11y
@@ -46,8 +47,13 @@ | |||
> | |||
<h2>Our Partner</h2> | |||
<p>{data.project.nonprofitDescription}</p> | |||
<Button | |||
href={data.project.nonprofitUrl} | |||
type="secondary-custom" |
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 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.
With this primary button you can use the primary-custom
type and change the background color + text color to make it look like the scheme in DoubleBanner
. Is that what you had in mind @SirajChokshi? The project-specific coloring for this looks pretty good imo.
My only concern with using the primary button here is that we'll eventually want a "visit site"/"view project" button that should be more visible, but that could have unique styling anyway.
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.
Yup, I meant primary-custom
. I like the project-themed colors too :)
I think the secondary button in this type of lockup should be just text. At least to me, the border on the secondary button is too heavily weighted to make the label readable.
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.
Great work adding this, and nice job incorporating the project-specific colors. I didn't really consider that before, but it makes total sense.
Just the comments in the thread above I think, + do you mind adding back the newline between the sections on line 50, just to improve the readability of the page file?
This button is primary, but I was thinking we'd have the project color as the background and white as the text color (cc @AndrewLester) |
Status: 🚀 Ready
Description
Adds a "Learn More" button that leads to the nonprofit's website in each H4I project's page
Fixes #85
Todos
Screenshots