Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add SponsorLogos section above footer #99

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add SponsorLogos section above footer #99

wants to merge 7 commits into from

Conversation

brandonmcconnell
Copy link
Contributor

@brandonmcconnell brandonmcconnell commented Oct 11, 2022 •
edited
Loading

Describe your changes

This PR adds a Featured Sponsors section to the bottom above the footer bar

The added logo gris is…

  • naturally responsive
  • centered (originally started with CSS grid but opted for flexbox for native centering on the last row)
  • accessible (titles and alt text added where applicable)
  • dynamic
    • href linking is optional
    • src image prop can be internal image (from ../images/sponsor-logos/) or external (http...)
    • includes transition to highlight hovered logo and show sponsor name (see screen recording below)

Link to issue this resolves

Fixes #33

Screenshot of changes(if relevant)

screen-recording.mov
  • The title should be something descriptive about what your changes do. (It will default to whatever you put as your commit message.)
This description should include :
  • A short summary of your changes
  • A link to the issue that relates to these changes
  • A screenshot of the changes if possible!

@vercel
Copy link

vercel bot commented Oct 11, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @brittanyjoiner15 on Vercel.

@brittanyjoiner15 first needs to authorize it.

@brandonmcconnell brandonmcconnell mentioned this pull request Oct 11, 2022
Closed
brittanyjoiner15
brittanyjoiner15 requested changes Oct 14, 2022
View reviewed changes
Copy link
Owner

@brittanyjoiner15 brittanyjoiner15 left a comment

Choose a reason for hiding this comment

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

@brandonmcconnell overall this looks amazing! I love the functionality and the styles. A couple things that need to be changed:

  1. i'd prefer we keep the styles in app.css or create other CSS files rather than inline.

  2. for some reason the app crashes when you click on the toggle to change from light to dark and vice versa. I don't see this happening on main, so i think it might be just this branch? Could you debug that a bit? I'm wondering if it has something to do with your useHover function, but also i don't really know why that would interfere with it.

src/components/SponsorLogos.jsx Outdated
const [hoverRef, isHovered] = useHover();
const borderWidth = '2px';
return (
<div ref={hoverRef} style={{
Copy link
Owner

Choose a reason for hiding this comment

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

Can we set these styles in either app.css or create another CSS file to store these styles?

src/components/SponsorLogos.jsx Outdated
src={src.startsWith('http') ? src : logo}
alt={name}
title={name}
style={{
Copy link
Owner

Choose a reason for hiding this comment

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

same note about these styles

src/components/SponsorLogos.jsx Outdated
padding: '1rem',
}}
/>
<div style={{
Copy link
Owner

Choose a reason for hiding this comment

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

and these

src/components/SponsorLogos.jsx Outdated
<EuiTitle size={titleSize}>
<h1 style={{ marginTop: '12px', marginBottom: '24px' }} className="eui-textCenter">Featured Sponsors</h1>
</EuiTitle>
<EuiFlexItem style={{
Copy link
Owner

Choose a reason for hiding this comment

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

also these styles should be moved to CSS file

src/components/SponsorLogos.jsx Outdated
>
<div style={{ maxWidth: '1200px' }}>
<EuiTitle size={titleSize}>
<h1 style={{ marginTop: '12px', marginBottom: '24px' }} className="eui-textCenter">Featured Sponsors</h1>
Copy link
Owner

Choose a reason for hiding this comment

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

these styles too

src/hooks/hover.js Outdated
@@ -0,0 +1,24 @@
// source: https://usehooks.com/useHover/
Copy link
Owner

Choose a reason for hiding this comment

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

@brandonmcconnell this could be that you are much more senior than me and there is a good reason for all of this, but are we sure this is really necessary for implementing the hover effect? Feel like there should be a simpler way to do it with the styles, but perhaps not actually. I will defer to you on this, but just wanted to comment and check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. Using plain CSS< this will be much simpler, so I'll remove this hook when I move the styles over to pure CSS 👍🏼

src/components/Footer.jsx
@@ -24,7 +24,7 @@ import { getColorTheme } from "../utilities/colors";

// sign-up data lives in this spreadsheet: https://docs.google.com/spreadsheets/d/1XgyHXaReTZ3Nq_r7QS18GDvqK_ht010QqnI6PXAnePA/edit#gid=1988825686

export default (props) => {
Copy link
Owner

Choose a reason for hiding this comment

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

@bpvcode what's the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESLint was complaining about export an unnamed arrow function. The usage of this component should still be identical. This just resolved the ESLint warning.

@brandonmcconnell brandonmcconnell mentioned this pull request Oct 14, 2022
Closed
3 tasks
@brittanyjoiner15
Copy link
Owner

@brandonmcconnell i see you've made changes in here! Is this ready for review? If so, can you resolve the merge conflicts and then let me know and ill give it another look and hopefully merge it!

@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Oct 20, 2022 •
edited
Loading

@brittanyjoiner15 Not quite yet! It's almost completely there, but I still haven't been able to figure out why the site freezes when the theme switcher is toggled. I'll leave an update here by Monday (if not sooner), if that's alright.

@brittanyjoiner15
Copy link
Owner

@brandonmcconnell no worries! Monday would be great!

@brittanyjoiner15 brittanyjoiner15 mentioned this pull request Oct 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brittanyjoiner15 brittanyjoiner15 brittanyjoiner15 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a sponsor section
2 participants
@brandonmcconnell @brittanyjoiner15