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

Implementation of new About Us page with team data and navigation(footer bar) #89

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

Conversation

jsi19
Copy link

@jsi19 jsi19 commented Jun 1, 2023

Description

  • Implemented new About Us page with brief description of Hotseat along with founders/rest of team. Each person on team is shown with an icon picture of themselves, along with their name and position on Hotseat.

  • Added navigation to footer as well

Deployment

  • I am making a frontend change, which will be auto-deployed via Heroku.

Added screenshots as visuals

Screenshot 2023-05-31 at 11 43 20 PM Screenshot 2023-05-31 at 11 45 09 PM Screenshot 2023-05-31 at 11 45 27 PM

@jsi19 jsi19 requested a review from nathanmsmith June 1, 2023 06:50
Copy link
Member

@nathanmsmith nathanmsmith left a comment

Choose a reason for hiding this comment

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

Great start! I've added a few comments on how we can better load images and style the page. Let me know if I can help with anything.

@@ -16,6 +16,9 @@
<div class="px-5 py-2">
<%= link_to "Privacy", page_path("privacy"), class: "footer-link" %>
</div>
<div class="px-5 py-2">
<%= link_to "About Us", page_path("about"), class: "footer-link" %>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Mind just indenting lines 20 and 21 to match the formatting of the other <div>s here?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed!

text-align: center;
}

/* CSS for the team icons */
Copy link
Member

Choose a reason for hiding this comment

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

We should be using TailwindCSS instead of defining custom styles here! Tailwind helps us by allowing us to more easily maintain our CSS and keep us on a standardized design framework without custom margin sizes, font weights, etc.

All of these CSS classes should be inlined using TailwindCSS classes. If you find yourself repeating yourself more than once, it might make sense to extract out the component into a Rails/ERB partial. Let me know if you need help here and I'm happy to go over it with you!

Here's an example for section-title. Note that I think you can remove the wrapping <div>.

<h2 class="text-center mb-10 font-bold text-2xl">Founders</h2>

Copy link
Author

Choose a reason for hiding this comment

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

I have added inline TailwindCSS for all components except for the team icon component as I am currently trying to figure out the img_tag first

@@ -130,19 +130,6 @@ CREATE TYPE public.weekly_time_type AS ENUM (
);


--
Copy link
Member

Choose a reason for hiding this comment

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

These changes are unrelated. I think they should go away if you run rails db:migrate. You can also git rm this file to make sure it doesn't show up in your PR!

</div>
</div>
<div class="team-component">
<div class="team-icon" style="background-image: url('https://drive.google.com/uc?export=view&id=1nCmo49fj2mZDd_MXFDBPi9o4-UK8ryiv')"></div>
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this way of doing images for a couple of reasons.

  1. It requires JavaScript. We shouldn't need to use JavaScript just to show some images!
  2. All of the images are host on Google Drive. What happens if there are Google Drive issues in the future and these images are deleted?

Instead what I'd do is download each image, crop them to a standard size (maybe like 500x500?), compress them with ImageOptim to get them down to a manageable file size, create a new public/images/team/ directory, put all of the optimized photos in there, and load them via Rails' image_tag helper. Let me know if you need help here!

Copy link
Author

Choose a reason for hiding this comment

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

I believe I have figured out the image_tag but the only thing now is that the images are too big, no matter how much of the tailwind css I change. I have not cropped them yet so maybe that is the issue. Will continue working on it soon

<div class="team-component">
<div class="team-icon" style="background-image: url('https://drive.google.com/uc?export=view&id=1nhOAeFLPEP1-gccQykSnJJgdU7KNuuxu')"></div>
<div class="team-details">
Katherine Chan <br>
Copy link
Member

Choose a reason for hiding this comment

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

Generally we only want to use <br>s in text, not for styling like this. Mind putting each line in a <div> and then using Tailwind to style?

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.

2 participants