UI: Created Navbar Component #230

stableapple · 2021-04-16T15:30:51Z

Description

Added Navbar Component

Fixes #208

Type of Change:

Code
User Interface

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials

Code/Quality Assurance Only

My changes generate no new warnings

decon-harsh

@stableapple According to the mocks there should be Blogs after FAQs, also Contact should be Contact Us.

src/Navigation.js

shades-7

It will be better if you will add a new gif in your PR with all the changes you have made.

src/Navigation.js

shades-7 · 2021-04-26T04:08:09Z

src/Navigation.js

+                          <li className="li_signIn">Sign In</li>
+                      </Link>
+                      <Link to="/" className="li">
+                          <li className="active" >Sign Up</li>


hello, I guess you have applied to the Active class only to Sign UP. I believe all the list elements in nav will have that active class on hover.
Apart from that things look good to me.

Am I right @Rahulm2310 @mtreacy002 ?

@shades-7 not sure about it. @VectoTec @Vuyanzi can you please tell what's the expected behaviour.

@shades-7 According to the Figma file, a blue line appears upon hover. Active class is for the blue background of the signup button.

Here you can link to the existing Register page until we have the new Sign Up page created so not to block the contributor to access the Register feature.

Rahulm2310 · 2021-04-26T07:26:22Z

@stableapple your design has a light blue bar on the top of navbar.

It is not present in the original navbar design.

src/Nav.css

Rahulm2310 · 2021-04-27T07:36:35Z

As we don't have the designs for smaller screens, can't say about the hamburger button and the nav. @stableapple Can you please show the behaviour(toggling) of nav on smaller screens(mobile) in a GIF.

stableapple · 2021-04-27T12:13:23Z

I was facing some issues with the toggling of the navbar. I resolved it. But I still couldn't figure out the shifting of the hamburger icon when clicked.

Vuyanzi · 2021-05-05T15:31:57Z

@mtreacy002 which section should I review exactly?

Vuyanzi · 2021-05-05T15:32:00Z

@mtreacy002 which section should I review exactly?

mtreacy002 · 2021-05-05T20:20:43Z

@Vuyanzi, just want to confirm the design done here is as expected. The Figma did show the different colour styles. But I'm curious to few things:

whether the Sign Up tab is going to have a permanent blue solid background regardless of the active page
whether the Sign In tab is going to have a permanent blue colour font regardless of the active page
at the moment, on hover the tab will change to have blue underline, how are we going to show which page the user is currently at (active tab)?

mtreacy002

@stableapple , sorry for taking a while to give my feedback. This is because I'm waiting for the sidebar to be created first or at least in progress, since otherwise contributors access to the existing features (upon login) will be blocked. Now that we have #254 in progress, hopefully we can avoid the blockage if we can finish this pr and #254 around the same time. Hope this make sense. I really appreciate your patience ❤️ .
On this point, please check my comments below 😉.

src/Navigation.js

mtreacy002 · 2021-06-17T13:27:41Z

src/Navigation.js

+               <Nav className="ml-auto">
+
+                      <Link to="/" className="li">
+                         <li >About Us</li>


This later can be linked to the video section (UI not completed yet, so no changes needed here 😉)

mtreacy002 · 2021-06-17T13:33:31Z

src/Navigation.js

+                      <Link to="/" className="li">
+                         <li >About Us</li>
+                      </Link>
+                      <Link to="/" className="li">


Based on Figma, this later can be linked to the Why BridgeInTech section (pr #247 is still in progress). No changes needed atm.

src/Navigation.js

mtreacy002 · 2021-06-17T14:11:42Z

src/Navigation.js

+                          <li className="li_signIn">Sign In</li>
+                      </Link>
+                      <Link to="/" className="li">
+                          <li className="active" >Sign Up</li>


Here you can link to the existing Register page until we have the new Sign Up page created so not to block the contributor to access the Register feature.

mtreacy002 · 2021-06-17T14:25:08Z

src/Navigation.js

-                        <Nav.Item>
-                            <Card>
-                                <Card.Header>
-                                    <Accordion.Toggle as={Link} to="/" eventKey="0" onClick={logout}>Logout</Accordion.Toggle>


Make sure this Logout option is shown for users who have successfully logged-in (instead of Sign In and Sign Up buttons).

mtreacy002 · 2021-06-17T14:44:55Z

Also, @stableapple . I've just asked @Vuyanzi on Zulip about the sequence of the main nav menus. Please don't hesitate to share with us your thoughts by replying to that post on the zulip channel 😉

…h-web into ui

stableapple · 2021-06-18T16:28:03Z

Should I implement the functionality around login and register in the current design or is there a separate design for it?

mtreacy002

@stableapple , thank you for adding the routes to the sections that have already completed. About the Sign In and Sign Up designs, for now you can just link these menu items to the existing login and register pages. This is so at least the contributors can access the existing features of Login/Register and continue with the development. Later when the Sign In and Sign Up prs #248 #249 are completed, we can refactor the pages to the new designs.

src/Navigation.js

mtreacy002

Thank you for accommodating to the changes requested, @stableapple. This looks good to me. The Login/Register and Logout features are now functioning as normal. I've also tested this on my local device and here's the outcome:

Code review: done
Tested on all possible behaviours

Expected behaviours:

Most of the menu items (not including Sign In/Sign Up) are linked to the relevant sections of the homepage (these menus are: FAQs, Blogs, Contact Us).
Sign In tab is linked to the Login page
Sign Up tab is linked to the Register page
user can sign in to existing account/register a new account
when user sign in the Sign In/Sign Up tabs are replaced by Logout tab

Actual behaviours: as expected (see gif below)

Environment:
OS: MacOS BigSur
Browser: Microsoft Edge

Thank you again for your patience and contribution 🎉🎉🎉. Now we just need one more reviewer to approve this then we'll merge it 😉. Perhaps @Rahulm2310 or @vj-codes can help?

test failing with unused var as the cause

mtreacy002

Sorry, I overlooked the testing that failed. Please remove user from the AuthContext call then it should fix the test. Thanks

src/Navigation.js

mtreacy002

LGTM @stableapple 🎉🎉🎉.

Rahulm2310 · 2021-06-25T04:31:21Z

Sorry, I was unable to review it. Anyway, great work @stableapple @mtreacy002 🎉 🎉

Created Navbar

76ab040

codesankalp added Open Source Hack Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 16, 2021

decon-harsh reviewed Apr 17, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 17, 2021

Added missing elements in navbar

571dfb9

shades-7 reviewed Apr 17, 2021

View reviewed changes

Rahulm2310 reviewed Apr 21, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

Rahulm2310 reviewed Apr 21, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

Added class to links

1a3a31d

devkapilbansal added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Apr 23, 2021

shades-7 suggested changes Apr 24, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

Fixed typo

1d7fb09

shades-7 suggested changes Apr 26, 2021

View reviewed changes

Rahulm2310 reviewed Apr 26, 2021

View reviewed changes

src/Nav.css Outdated Show resolved Hide resolved

Rahulm2310 reviewed Apr 26, 2021

View reviewed changes

src/Nav.css Outdated Show resolved Hide resolved

Rahulm2310 added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 26, 2021

Rahulm2310 reviewed Apr 26, 2021

View reviewed changes

src/Nav.css Outdated Show resolved Hide resolved

Removed the space in sign up button

5ed569d

mtreacy002 requested a review from Rahulm2310 April 27, 2021 04:56

mtreacy002 added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Apr 27, 2021

Resolved bug in toggle

9b9fbf4

Fixed button issue

eba63b4

mtreacy002 assigned stableapple May 2, 2021

isabelcosta changed the base branch from develop to homepage-navigation May 23, 2021 11:51

isabelcosta changed the base branch from homepage-navigation to develop May 23, 2021 11:56

mtreacy002 mentioned this pull request May 23, 2021

UI: Temporary side navigation for protected pages #250

Closed

mtreacy002 requested changes Jun 17, 2021

View reviewed changes

mtreacy002 added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jun 17, 2021

stableapple added 3 commits June 17, 2021 21:38

Merge branch 'develop' of https://github.com/anitab-org/bridge-in-tec…

4c5a8fb

…h-web into ui

Added routes

0bb9019

Added register login routes

8321a2d

mtreacy002 requested changes Jun 19, 2021

View reviewed changes

src/Navigation.js Show resolved Hide resolved

Added AuthContext

ab838d6

mtreacy002 previously approved these changes Jun 20, 2021

View reviewed changes

mtreacy002 requested changes Jun 20, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

Removed user

2044437

mtreacy002 reviewed Jun 20, 2021

View reviewed changes

src/Navigation.js Outdated Show resolved Hide resolved

Remove Accordian

f2e2fb9

mtreacy002 approved these changes Jun 20, 2021

View reviewed changes

mtreacy002 requested review from Rahulm2310 and removed request for Rahulm2310 June 20, 2021 14:02

mtreacy002 added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Jun 21, 2021

mtreacy002 merged commit 334ba9b into anitab-org:develop Jun 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Created Navbar Component #230

UI: Created Navbar Component #230

stableapple commented Apr 16, 2021 •

edited

Loading

decon-harsh left a comment

shades-7 left a comment

shades-7 Apr 26, 2021

Rahulm2310 Apr 26, 2021

stableapple Apr 26, 2021

mtreacy002 Jun 17, 2021

Rahulm2310 commented Apr 26, 2021

Rahulm2310 commented Apr 27, 2021

stableapple commented Apr 27, 2021

Vuyanzi commented May 5, 2021

Vuyanzi commented May 5, 2021

mtreacy002 commented May 5, 2021 •

edited

Loading

mtreacy002 left a comment

mtreacy002 Jun 17, 2021

mtreacy002 Jun 17, 2021

mtreacy002 Jun 17, 2021 •

edited

Loading

mtreacy002 Jun 17, 2021

mtreacy002 Jun 17, 2021

mtreacy002 commented Jun 17, 2021

stableapple commented Jun 18, 2021

mtreacy002 left a comment

mtreacy002 left a comment

mtreacy002 left a comment

mtreacy002 left a comment

Rahulm2310 commented Jun 25, 2021

UI: Created Navbar Component #230

UI: Created Navbar Component #230

Conversation

stableapple commented Apr 16, 2021 • edited Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

decon-harsh left a comment

Choose a reason for hiding this comment

shades-7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahulm2310 commented Apr 26, 2021

Rahulm2310 commented Apr 27, 2021

stableapple commented Apr 27, 2021

Vuyanzi commented May 5, 2021

Vuyanzi commented May 5, 2021

mtreacy002 commented May 5, 2021 • edited Loading

mtreacy002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Jun 17, 2021

stableapple commented Jun 18, 2021

mtreacy002 left a comment

Choose a reason for hiding this comment

mtreacy002 left a comment

Choose a reason for hiding this comment

mtreacy002 left a comment

Choose a reason for hiding this comment

mtreacy002 left a comment

Choose a reason for hiding this comment

Rahulm2310 commented Jun 25, 2021

stableapple commented Apr 16, 2021 •

edited

Loading

mtreacy002 commented May 5, 2021 •

edited

Loading

mtreacy002 Jun 17, 2021 •

edited

Loading