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

feat: add side nav and 404 page #29

Merged
merged 8 commits into from
Nov 7, 2024
Merged

feat: add side nav and 404 page #29

merged 8 commits into from
Nov 7, 2024

Conversation

limsohee1002
Copy link

@limsohee1002 limsohee1002 commented Nov 5, 2024

Description

  • add side nav
  • add side nav api
  • add local storage to save side nav data
  • add 404 page

Type(s) of changes

  • Bug fix
  • New feature
  • Update to an existing feature

Motivation for PR

#26 #16

Applicable screenshots

https://www.loom.com/share/621e215a90fb4e9a9bce5bd50f060350?sid=958c8a51-a045-4cff-a1a5-d890e3f37f6c

@limsohee1002 limsohee1002 marked this pull request as ready for review November 6, 2024 16:54
@limsohee1002 limsohee1002 marked this pull request as draft November 6, 2024 17:10
@limsohee1002 limsohee1002 changed the title feat: add side nav api feat: add side nav and 404 page Nov 6, 2024
@limsohee1002 limsohee1002 marked this pull request as ready for review November 6, 2024 17:48
Copy link
Collaborator

@nahbee10 nahbee10 left a comment

Choose a reason for hiding this comment

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

This overlap issue still happening! Will document this on QA ticket, let's address this later in the QA phase.
Screenshot 2024-11-06 at 3 16 34 PM

I've added one comment about make the inactive category items are also clickable

Comment on lines 104 to 111
<h3
className={cn('subheading-2 my-3', {
'text-light-accent-1 dark:text-dark-accent-1': categoryisActive,
'text-light-neutral-2 dark:text-dark-neutral-2': !categoryisActive,
})}
>
{category.name}
</h3>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I updated it! on nav, I think we should go though hover state with designer. I felt like it is not consistant but I guess we can handle that on QA!

@nahbee10
Copy link
Collaborator

nahbee10 commented Nov 6, 2024

Also noticed that we need to adjust heading level which is used by right side nav('On this page') for jump links. For now it's using too much of body text I think:
Screenshot 2024-11-06 at 3 21 34 PM

@limsohee1002
Copy link
Author

Also noticed that we need to adjust heading level which is used by right side nav('On this page') for jump links. For now it's using too much of body text I think: Screenshot 2024-11-06 at 3 21 34 PM

right now, I am grabbing header 1 to 4 to add to side navigation. I think we should ask editor how they imagine to use that. If they would like us to only grab 1 to 3, we can do that as well, but I have seen some h4 that have different text sizes that are represented as titles, so let's confirm with the editor!
It is a super easy fix, if they want us to use h4 as side nav title, they should update this article so I think it will be good to confirm first

@limsohee1002 limsohee1002 requested a review from nahbee10 November 6, 2024 22:18
Copy link
Collaborator

@nahbee10 nahbee10 left a comment

Choose a reason for hiding this comment

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

We also need to add side nav items to the mobile nav modal(design). Let's handle it in the separate PR.
Screenshot 2024-11-06 at 9 44 45 PM

Copy link

@mokaymokay mokaymokay left a comment

Choose a reason for hiding this comment

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

Nice work! Just a couple small things. No additional comments besides Nahee's

Comment on lines +98 to +100
{
hidden: i === 0,
}

Choose a reason for hiding this comment

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

nit: you can prob just do first:hidden here so you don't need to use js logic?

Copy link
Author

Choose a reason for hiding this comment

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

It is not actually first element inside of the div 🥲 it has button div above it so I just added that ☺️

<a href={category.url}>
<h3
className={cn(
'transitoin subheading-2 my-3 hover:text-light-accent-1 hover:dark:text-dark-accent-1',

Choose a reason for hiding this comment

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

typo: transition

@limsohee1002 limsohee1002 merged commit afba82d into master Nov 7, 2024
3 checks passed
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.

3 participants