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.

Sign up for GitHub

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

Jump to bottom

Admin create posting side "navigation bar" #137

Merged
merged 11 commits into from
Feb 18, 2022
Merged

Admin create posting side "navigation bar" #137

merged 11 commits into from
Feb 18, 2022

Conversation

hujoseph99
Copy link
Contributor

Ticket link

Closes #60

Implementation description

  • Used library chakra-ui-steps to help facilitate this
  • Created an extensible component that will create steps for us based on an array of labels passed in

Steps to test

  1. Open up the app, then go to the "Side Navbar Demo" in the navbar at the top of the page

What should reviewers focus on?

  • The styling of the component (to see if there's anything that I missed)
  • The interface provided for the user

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@hujoseph99 hujoseph99 requested a review from a team February 7, 2022 23:17
LenaNguyen
LenaNguyen requested changes Feb 8, 2022
View reviewed changes
Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

Just a couple of questions and we should wait for design to get back to us :)

frontend/src/App.tsx Outdated
@@ -2,6 +2,7 @@ import "@fontsource/inter/400.css";
import "@fontsource/inter/700.css";
import "@fontsource/raleway/400.css";
import "@fontsource/raleway/600.css";
import "@fontsource/open-sans/700.css";
Copy link
Member

Choose a reason for hiding this comment

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

I see that the number of the step uses the font open-sans. Let's double check with @krystaltruong @sohak22 @amandatdu @JoshuaYe that this is correct. I feel like we should just use one of the fonts that we're already using in our design system (i.e. raleway or inter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I was just going off of what was in the mockup on Figma.

Copy link
Member

Choose a reason for hiding this comment

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

did you get a chance to ask designers about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry I thought they would look at this PR. I'll check with them next work session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In regards to this, I talked with @krystaltruong and we agreed to go with using the Inter font to stay consistent with our design system. I've made the appropriate changes

frontend/src/theme/index.ts Outdated Show resolved Hide resolved
frontend/src/theme/stepsTheme.ts Show resolved Hide resolved
frontend/src/App.tsx Show resolved Hide resolved
frontend/src/components/pages/SideNavbarDemo.tsx Show resolved Hide resolved
@hujoseph99 hujoseph99 force-pushed the joseph/admin-posting-side-navbar branch from c123594 to 32625c0 Compare February 13, 2022 21:52
@github-actions
Copy link

github-actions bot commented Feb 13, 2022 •
edited
Loading

Visit the preview URL for this PR (updated for commit 5e78475):

https://sistering-dev--pr137-joseph-admin-posting-ql0moxqf.web.app

(expires Fri, 25 Feb 2022 02:02:00 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

LenaNguyen
LenaNguyen reviewed Feb 14, 2022
View reviewed changes
Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

Everything looks great! Just wanna follow up on the what to do about the Open Sans font and then I'll approve

@hujoseph99 hujoseph99 force-pushed the joseph/admin-posting-side-navbar branch from 32625c0 to 5e78475 Compare February 18, 2022 01:57
LenaNguyen
LenaNguyen approved these changes Feb 18, 2022
View reviewed changes
Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

TY for following up with design!

@hujoseph99 hujoseph99 merged commit 19c956c into main Feb 18, 2022
@hujoseph99 hujoseph99 deleted the joseph/admin-posting-side-navbar branch February 18, 2022 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LenaNguyen LenaNguyen LenaNguyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Admin create posting side "navigation bar" (roadmap)
2 participants
@hujoseph99 @LenaNguyen