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

Learn Area Footer Redesign #1645

Merged
merged 7 commits into from Oct 27, 2023
Merged

Learn Area Footer Redesign #1645

merged 7 commits into from Oct 27, 2023

Conversation

Oluwabukolab
Copy link
Contributor

@Oluwabukolab Oluwabukolab commented Oct 15, 2023

This fixes #1591.

@gpetiot gpetiot added the outreachy Outreachy contributions and blog posts label Oct 16, 2023
@sabine
Copy link
Collaborator

sabine commented Oct 20, 2023

Hey @sophiatunji! This is really nice, the only two things left to do seem

  1. to add Tailwind styles, and
  2. to move the secondary footer in package_layout.eml to the main content area.

@Oluwabukolab
Copy link
Contributor Author

Thank you for the review Sabine! Will make the necessary changes! :)

@Oluwabukolab Oluwabukolab marked this pull request as ready for review October 20, 2023 22:40
@Oluwabukolab
Copy link
Contributor Author

My PR is ready for review, cc @sabine @SaySayo

Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

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

Hey @sophiatunji, that looks really good already! 👍 Here's some feedback:

  1. Policies section should not be fixed to the bottom. This is just a part of the non-sticky footer.
  2. On small screens (320px): We need to figure out how to collapse this in a way that works. Policies section is too wide to be a single row. Resources section seems to have whitespace right of it that looks empty, while the left column seems very compressed.
    Screenshot 2023-10-21 at 12-35-49 Installing OCaml · OCaml Tutorials
  3. whitespace between OCaml Logo and "Innovation. Community. Security." seems slightly larger in the design than in the implementation.
  4. In the design, there is a small spacing above the section headings that this implementation doesn't have. See Figma:
    Screenshot from 2023-10-21 12-52-34

Comment on lines 16 to 18
<section class="flex justify-between w-[80%]">
<div class="flex flex-col gap-4">
<div class="flex flex-col gap-4">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot from 2023-10-21 12-45-34

Click area for links is unnecessarily small. Generally, when you have links in a navigation, for the sake of people being able to click on them easily, use padding instead of gap. Here, py-2.5 for the a links seems to be closest to the design.

src/ocamlorg_frontend/components/secondary_footer.eml Outdated Show resolved Hide resolved
@Oluwabukolab
Copy link
Contributor Author

the about and resources have a uniform space between them
Screenshot 2023-10-22 at 21 12 54

and I have made the remaining corrections. @sabine @SaySayo

sabine added a commit that referenced this pull request Oct 24, 2023
@sabine
Copy link
Collaborator

sabine commented Oct 24, 2023

Great, I'll put this on staging for our designer to review! 👍

@Clairevanden
Copy link
Contributor

Hello @sophiatunji , it is Claire the designer, what you have done looks great. I just have 2 small recommendations to make. You can have a look to my screen shot attach. Let me know if you have any questions in those! Thanks
Screenshot 2023-10-24 at 17 11 45

@Oluwabukolab
Copy link
Contributor Author

Thanks for the review! I have made the recommended adjustments. cc @SaySayo @sabine @Clairevanden

Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

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

Hey @sophiatunji, great work, here's some detail adjustments.

I figured a grid column layout will probably collapse better here (just like the existing footer on the other pages) and adjusted some paddings and spacings.

@@ -114,8 +114,9 @@ Layout.base
<%s! left_sidebar_html %>
</div>

<div class="flex-1 z-0 z- min-w-0 pt-6 pb-12 md:pb-[70vh]">
<div class="flex-1 z-0 z- min-w-0 pt-6 pb-12">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div class="flex-1 z-0 z- min-w-0 pt-6 pb-12">
<div class="flex-1 z-0 z- min-w-0 pt-6">

remove bottom padding so we can't scroll past the footer

@@ -121,8 +121,9 @@ inner_html
<%s! Option.value ~default:"" left_sidebar_html %>
</div>

<div class="flex-1 z-0 z- min-w-0 lg:pt-6 pb-12 lg:pb-[70vh] <%s! if right_sidebar_html != None then "lg:max-w-3xl" else "" %>">
<div class="flex-1 z-0 z- min-w-0 lg:pt-6 pb-12 <%s! if right_sidebar_html != None then "lg:max-w-3xl" else "" %>">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div class="flex-1 z-0 z- min-w-0 lg:pt-6 pb-12 <%s! if right_sidebar_html != None then "lg:max-w-3xl" else "" %>">
<div class="flex-1 z-0 z- min-w-0 lg:pt-6 <%s! if right_sidebar_html != None then "lg:max-w-3xl" else "" %>">

Comment on lines 55 to 58
<a href="<%s Url.carbon_footprint %>" class="font-normal text-lighter leading-7 py-1">Carbon Footprint</a>
<a href="<%s Url.governance %>" class="font-normal text-lighter leading-7 py-1">Governance</a>
<a href="<%s Url.privacy_policy %>" class="font-normal text-lighter leading-7 py-1">Privacy</a>
<a href="<%s Url.code_of_conduct %>" class="font-normal text-lighter leading-7 py-1">Code Of Conduct</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<a href="<%s Url.carbon_footprint %>" class="font-normal text-lighter leading-7 py-1">Carbon Footprint</a>
<a href="<%s Url.governance %>" class="font-normal text-lighter leading-7 py-1">Governance</a>
<a href="<%s Url.privacy_policy %>" class="font-normal text-lighter leading-7 py-1">Privacy</a>
<a href="<%s Url.code_of_conduct %>" class="font-normal text-lighter leading-7 py-1">Code Of Conduct</a>
<a href="<%s Url.carbon_footprint %>" class="font-normal text-lighter leading-7 p-2.5">Carbon Footprint</a>
<a href="<%s Url.governance %>" class="font-normal text-lighter leading-7 p-2.5">Governance</a>
<a href="<%s Url.privacy_policy %>" class="font-normal text-lighter leading-7 p-2.5">Privacy</a>
<a href="<%s Url.code_of_conduct %>" class="font-normal text-lighter leading-7 p-2.5">Code Of Conduct</a>

</div>
</footer>
<div
class="flex flex-wrap gap-x-7 md:justify-between items-center py-4 border-solid border-t-[1px] border-[#00000033] bottom-0 bg-white">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class="flex flex-wrap gap-x-7 md:justify-between items-center py-4 border-solid border-t-[1px] border-[#00000033] bottom-0 bg-white">
class="flex flex-wrap gap-x-7 md:justify-between items-center py-2 border-solid border-t-[1px] border-[#00000033] bottom-0 bg-white">

Comment on lines 40 to 48
<a href="<%s Url.install %>" class="font-normal text-lighter leading-7 py-1">Install OCaml</a>
<a href="<%s Url.getting_started %>" class="font-normal text-lighter leading-7 py-1">Get Started</a>
<a href="<%s Url.platform %>" class="font-normal text-lighter leading-7 py-1">Platform Tools</a>
<a href="<%s Url.manual %>" class="font-normal text-lighter leading-7 py-1">Language Manual</a>
<a href="<%s Url.api %>" class="font-normal text-lighter leading-7 py-1">Standard Library API</a>
<a href="<%s Url.books %>" class="font-normal text-lighter leading-7 py-1">Books</a>
<a href="<%s Url.exercises %>" class="font-normal text-lighter leading-7 py-1">Exercises</a>
<a href="<%s Url.papers %>" class="font-normal text-lighter leading-7 py-1">Papers</a>
<a href="<%s Url.playground %>" class="font-normal text-lighter leading-7 py-1">OCaml Playground</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<a href="<%s Url.install %>" class="font-normal text-lighter leading-7 py-1">Install OCaml</a>
<a href="<%s Url.getting_started %>" class="font-normal text-lighter leading-7 py-1">Get Started</a>
<a href="<%s Url.platform %>" class="font-normal text-lighter leading-7 py-1">Platform Tools</a>
<a href="<%s Url.manual %>" class="font-normal text-lighter leading-7 py-1">Language Manual</a>
<a href="<%s Url.api %>" class="font-normal text-lighter leading-7 py-1">Standard Library API</a>
<a href="<%s Url.books %>" class="font-normal text-lighter leading-7 py-1">Books</a>
<a href="<%s Url.exercises %>" class="font-normal text-lighter leading-7 py-1">Exercises</a>
<a href="<%s Url.papers %>" class="font-normal text-lighter leading-7 py-1">Papers</a>
<a href="<%s Url.playground %>" class="font-normal text-lighter leading-7 py-1">OCaml Playground</a>
<a href="<%s Url.install %>" class="font-normal text-lighter leading-7 py-2.5">Install OCaml</a>
<a href="<%s Url.getting_started %>" class="font-normal text-lighter leading-7 py-2.5">Get Started</a>
<a href="<%s Url.platform %>" class="font-normal text-lighter leading-7 py-2.5">Platform Tools</a>
<a href="<%s Url.manual %>" class="font-normal text-lighter leading-7 py-2.5">Language Manual</a>
<a href="<%s Url.api %>" class="font-normal text-lighter leading-7 py-2.5">Standard Library API</a>
<a href="<%s Url.books %>" class="font-normal text-lighter leading-7 py-2.5">Books</a>
<a href="<%s Url.exercises %>" class="font-normal text-lighter leading-7 py-2.5">Exercises</a>
<a href="<%s Url.papers %>" class="font-normal text-lighter leading-7 py-2.5">Papers</a>
<a href="<%s Url.playground %>" class="font-normal text-lighter leading-7 py-2.5">OCaml Playground</a>

<li>
<a class="h-10 md:h-12 flex items-center text-lighter" href="<%s Url.jobs %>" class="text-base leading-6 text-lighter">Jobs</a>
</li>
<div class="flex flex-col gap-2">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<div class="flex flex-col gap-2">
<div class="flex flex-col">

src/ocamlorg_frontend/components/secondary_footer.eml Outdated Show resolved Hide resolved
src/ocamlorg_frontend/components/secondary_footer.eml Outdated Show resolved Hide resolved
src/ocamlorg_frontend/components/secondary_footer.eml Outdated Show resolved Hide resolved
sabine added a commit that referenced this pull request Oct 26, 2023
Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

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

Thank you @sophiatunji for the contribution, this is ready to merge now! 👍

@sabine sabine merged commit 9090bff into ocaml:main Oct 27, 2023
3 checks passed
cuihtlauac pushed a commit that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
outreachy Outreachy contributions and blog posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a New Footer for the Learn Area
4 participants