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

107 Broken Members Dropdown #115

Merged
merged 3 commits into from
Sep 3, 2024
Merged

107 Broken Members Dropdown #115

merged 3 commits into from
Sep 3, 2024

Conversation

kk7-py
Copy link
Contributor

@kk7-py kk7-py commented Aug 20, 2024

Context

  • Fixed broken dropdown menu, years are now aligned vertically
  • Years are also sorted in a descending order

Closes #107

What Changed?

  • Removed and replaced sorting logic by converting years into a number and sorting the value in a descending order
  • Fixed dropdown menu by aligning the years vertically.
  • Centred the years in dropdown menu and slightly adjusted the padding to fit the years better.

How To Review

  • Hover over the members heading on the website to see the hidden dropdown menu, and check if the years are aligned vertically and in descending order.

  • Checkout
    next/components/header/header.tsx
    to view the changes.

  • Review implemented sorting logic and styles

Copy link
Contributor

@Oculux314 Oculux314 left a comment

Choose a reason for hiding this comment

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

Concise bugfix, detailed description, great naming and commit conventions, AND implementing additional error checking on your own initiative? Dang this is a really great PR. See one place where I think you can use tailwind rather than inline CSS (references provided!) - fix that and I'm happy to merge!

(P.s. I altered some confusing code from 2023 that was tangentially-related to your PR so do a git pull on this branch before changing anything 😄)

flexDirection: "column", // Stack the years vertically
alignItems: "center", // Center the text horizontally
padding: "0.25rem 0.5rem", // Adjust padding to fit the text better
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@Oculux314 Oculux314 left a comment

Choose a reason for hiding this comment

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

Lgtm thanks for those changes :)

Copy link

@YvonneLiew YvonneLiew left a comment

Choose a reason for hiding this comment

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

lgtm

@Oculux314
Copy link
Contributor

@kk7-py yo! Two approvals means you've got the go-ahead to merge this into main, GJ :D

@Oculux314 Oculux314 merged commit 07db029 into main Sep 3, 2024
@Oculux314 Oculux314 deleted the 107_broken_members_dropdown branch September 3, 2024 12:54
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.

Broken Members Dropdown
3 participants