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.

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

footer component #130

Merged
merged 5 commits into from
Dec 29, 2020
Merged

footer component #130

merged 5 commits into from
Dec 29, 2020

Conversation

tim-nguyen-cs
Copy link
Contributor

Problem

As a prospective hacker,
I would like to view additional footer resources
To resolve more inquiries and questions I have about CruzHacks.

Link to Asana Ticket

Solution

To implement the Footer section of the homepage, flexbox-based positioning and margin alignments were used to replicate the provided Figma design with responsiveness. Although preliminary social media links and other graphics have not been included yet, this first draft outlines boilerplate code regarding how the footer is to be implemented.

Change summary:

  • first iteration of footer f8fd139

Steps to Verify:

  1. Checkout branch using git checkout feature/footer
  2. Start local development server using yarn start
  3. Verify that footer section is designed and responds correctly.

@tim-nguyen-cs tim-nguyen-cs added the enhancement New feature or request label Dec 25, 2020
@tim-nguyen-cs tim-nguyen-cs requested a review from a team December 25, 2020 00:04
@tim-nguyen-cs tim-nguyen-cs self-assigned this Dec 25, 2020
Copy link
Member

@ejmockler ejmockler left a comment

Choose a reason for hiding this comment

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

looks good, just one general issue with the contrast here.. this is tied into the design, but maybe add text-shadow to add a bit more contrast
Screenshot from 2020-12-25 19-45-12

@tim-nguyen-cs
Copy link
Contributor Author

looks good, just one general issue with the contrast here.. this is tied into the design, but maybe add text-shadow to add a bit more contrast
Screenshot from 2020-12-25 19-45-12

Added font-shadow per your suggestions - it looks great! Thanks for the tip - let me know if there's anything else you want me to change!

Copy link
Member

@ilopezro ilopezro left a comment

Choose a reason for hiding this comment

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

I think this looks good overall! Please take a look at my comments & let me know what you think!

Comment on lines 19 to 21
a {
color: inherit;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we use class names instead of generic attributes? I understand why we have this here, but I don't see the a tag anywhere within Home index view. Let's give the a tags a classname within the Footer component and style the attributes there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's understandable - no problem!

@@ -0,0 +1,52 @@
import React from "react";
Copy link
Member

Choose a reason for hiding this comment

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

Because we upgraded to a new version of typescript in #71, we no longer have to import react if we don't use it. Instead, we can import it using the following since this is the convention that it is moving towards using this newer version of react:
import * as React from "react";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, cool ~ no problem! In that case, we might want to change this notation for all other files too!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- I believe I ran a command that changed all the existing import statements. This only appears once or twice in the codebase. I did a cmd + F on the existing codebase and it looks like I added the wrong import statement after the PR was merged in. I think we should address those in a separate PR. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I was lowkey planning a separate PR to just clean up the codebase a little bit, so that sounds like a good idea!

</div>
<div className="Footer__text--middle">
CruzHacks was formerly known as HackUCSC, and launched in April 2014.
The founders were&nbsp;
Copy link
Member

Choose a reason for hiding this comment

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

SUPER NIT: I'm honestly not that bit a fan of having &nbsp; directly in the codebase as it creates a bit of messy code. We can instead style the a tag accordingly . I'd prefer if we styled it using SCSS instead of hardcoding it into the text like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that these entities make code look a little unclean, but I'm not quite sure what about the resolution that you mentioned. The &nbsp; entities are here, because our linter requires that content within a tags be placed on a new line. Although I personally prefer inline a tags, this causes sentences to be partitioned weirdly and requires &nbsp; used in place of any trailing spaces that are pre-maturely truncated.

Let me know if this makes sense and how to proceed - also note that this section will be removed later when this sentence is integrated into our Teams or Sponsors page.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yes! I see what the issue is. Since the a tag has 3 props and contents within it, in order to maintain the character count for that line to 120, it makes the a go to a separate line. Adding a space at the end of the line before the a tag would cause the linter to cry since we don't want trailing spaces. In order to remove these entities, we can style it in the css by adding a margin-left: .5rem to the a tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, that sounds like a clever fix! I'll have that done by tonight!

Copy link
Member

@ejmockler ejmockler left a comment

Choose a reason for hiding this comment

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

looks good from here! let's stage this up

Copy link
Member

@ilopezro ilopezro left a comment

Choose a reason for hiding this comment

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

Dope work! 🚀

@tim-nguyen-cs
Copy link
Contributor Author

I'll touch this up again when other parts of our webpage get flushed out, but let's get this in!

@tim-nguyen-cs tim-nguyen-cs merged commit dd49d8a into main Dec 29, 2020
@tim-nguyen-cs tim-nguyen-cs deleted the feature/footer branch December 29, 2020 08:51
ilopezro pushed a commit that referenced this pull request Jan 1, 2021
* first iteration of footer

* revised component styling
@ilopezro ilopezro mentioned this pull request Jan 1, 2021
ilopezro pushed a commit that referenced this pull request Jan 3, 2021
* first iteration of footer

* revised component styling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants