Footer Component Added #228

naveen8801 · 2021-04-12T15:11:26Z

Description

New Footer component is added in Home folder:-

Added Footer.jsx and Footer.css in Home
Added Fonts (2 fonts) and icons ( 4 icons ) in assets folder

Fixes #215

Type of Change:

User Interface

Code/Quality Assurance Only

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

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
New and existing unit tests pass locally with my changes

shades-7 · 2021-04-12T17:58:51Z

@mtreacy002 This is the first time I am trying to test a PR. so let me know if i am doing it wrong.

This is my feedbacks:

All the social icons are directing to the same page.
Line spacing is more for this paragraph "We are known for connecting top talents with top comapnies. We make great possibilities happen."
In mobile view stacking is done but i think stacking need to de done in all the subsections too like explore

isabelcosta · 2021-04-12T23:19:52Z

@shades-7 this GIF you posted looks amazing 🤩 I also love the feedback you provided, is so specific! To make this awesome I would also suggest using our QA template, just to explain a little of what you tested. You can find the template at point 7 here https://github.com/anitab-org/documentation/blob/master/quality-assurance.md#how-to-test-a-pr

cc @rpattath 🤩

mtreacy002 · 2021-04-13T06:52:47Z

@shades-7 , I agree with @isabelcosta that you've done amazing work reviewing the PR. You've raised very good points on your feedback above. As our Github workflow would go from PR submission to PR review then PR testing, I would suggest you post your feedback as the PR review using the Request changes option (see screenshot below) so that @naveen8801 could give her response to your feedback and make necessary changes if required. This way, your pr review will also count towards OSH as what we consider as Top Participants are contributors who help Review other's PRs, test other's PRs, or help other participants in general 😉.

shades-7

All the social icons are directing to the same page but ideally, they should open the required sites.
Line spacing should be a little less for this paragraph "We are known for connecting top talents with top comapnies. We make great possibilities happen."
In mobile view, stacking is done but I think stacking need to de done in all the subsections too like explore, visit etc

naveen8801 · 2021-04-13T16:17:42Z

hi @shades-7 @mtreacy002 , where to get all links like social links , legal pages and explore sections ? :-)

Rahulm2310 · 2021-04-14T20:43:54Z

@mtreacy002 @isabelcosta for social links, we are going to add the links to AnitaB.org's social media links. right? Also, as we don't have the designs ready for legal pages i.e. Terms & Conditions, Privacy Policy, we can directly add links to these pages for now:

naveen8801 · 2021-04-15T15:39:29Z

hi @Rahulm2310 , i have added social links , let me know if there is some correction :-)

Rahulm2310 · 2021-04-15T16:48:57Z

@naveen8801 please wait for @mtreacy002 's point of view.

naveen8801 · 2021-04-15T16:49:53Z

@naveen8801 please wait for @mtreacy002 's point of view.

ok @Rahulm2310 👍

mtreacy002

@naveen8801, the page looks amazing 💘 . About the policies links, for now I think we can use the links that @Rahulm2310 's suggested. Although, we could also use the privacy modals @Rahulm2310 created on the register page to keep it consistent across the BridgeInTech application. However, once the issues on the documentation repo on creating PRIVACY_POLICY.md and TERMS_OF_USE.md are solved, we would change the policies links to those of the documentation repo. So, for now, IMO no need to use the privacy modals as per Register page, let's just use the links given on @Rahulm2310 post above. @naveen8801 can you please add these links? Thanks.

naveen8801 · 2021-04-17T05:01:31Z

@naveen8801, the page looks amazing 💘 . About the policies links, for now I think we can use the links that @Rahulm2310 's suggested. Although, we could also use the privacy modals @Rahulm2310 created on the register page to keep it consistent across the BridgeInTech application. However, once the issues on the documentation repo on creating PRIVACY_POLICY.md and TERMS_OF_USE.md are solved, we would change the policies links to those of the documentation repo. So, for now, IMO no need to use the privacy modals as per Register page, let's just use the links given on @Rahulm2310 post above. @naveen8801 can you please add these links? Thanks.

hi @mtreacy002 , i think i have already added above links (Privacy Policy and Terms of Use) in my last commit , please have a look :)

mtreacy002 · 2021-04-17T05:32:54Z

Ooops, my bad. Sorry @naveen8801 😄. Yup, you're right. The links to the privacy policy and terms of use are already there. Ok, will continue with the review now, 😉

naveen8801 · 2021-04-17T05:37:42Z

Ooops, my bad. Sorry @naveen8801 😄. Yup, you're right. The links to the privacy policy and terms of use are already there. Ok, will continue with the review now, 😉

No problem 👍😃 @mtreacy002

mtreacy002

@naveen8801 , I tried to run the application after pulling it from your pr branch, but although the application compiled successfully, I'm facing 2 issues:

I couldn't see the footer as per your gif. I only can see the existing homepage.

I noticed 6 warnings came up in relation to the newly created footer.

Could you please suggest what went wrong here?
Thanks beforehand 😉

naveen8801 · 2021-04-17T13:59:54Z

@naveen8801 , I tried to run the application after pulling it from your pr branch, but although the application compiled successfully, I'm facing 2 issues:

I couldn't see the footer as per your gif. I only can see the existing homepage.

I noticed 6 warnings came up in relation to the newly created footer.

Could you please suggest what went wrong here?
Thanks beforehand 😉

Hii @mtreacy002 , actually i removed Footer component from home page before creating this PR and also those warnings are due to use of target="_blank" without rel attribute and can be easily resolved by adding rel="noopener noreferrer" attribute in those anchor tags where target attribute is used , i have made the changes , now you can review 👍

mtreacy002

@naveen8801, the warnings have disappeared and I can see the footer now 🎉. I just have one minor request. Can you please set it so that homepage content is maintained (so not partially cut) by the footer?
This is also because later on when we added all the sctions of the Homepage as per the Figma file, the Footer needs to be fixed at the very bottom.

Thanks

Rahulm2310 · 2021-04-17T20:43:17Z

src/home/Footer.jsx

+                  alt="twitter_logo"
+                ></img>
+              </a>
+              <a href="https://www.facebook.com/AnitaBorgIndia/" target="_blank" rel="noopener noreferrer">


Why this is linked to AntaBorgIndia 's facebook page? It should be linked to AnitaB's global facebook page.

Rahulm2310 · 2021-04-17T20:43:51Z

src/home/Footer.jsx

+                  alt="gihub_logo"
+                ></img>
+              </a>
+              <a href="https://www.linkedin.com/company/anitaborg-india/" target="_blank" rel="noopener noreferrer">


It should also be linked to AnitaB's global linkedin page.

OK @Rahulm2310 👍

Rahulm2310 · 2021-04-17T20:46:06Z

src/home/Footer.jsx

+            </ul>
+          </div>
+          <div className="flexbox-item">
+            <h3 className="heading">Visit</h3>


I think this section can be removed as we don't have a visit location.

Hi @Rahulm2310 @mtreacy002 , should i remove this section ?

@naveen8801 , I believe @Rahulm2310 has answer your question so you can remove this 😉. @Vuyanzi and @isabelcosta unless you'd prefer this Visit to show AnitaB.Org mailing address as per anitab.org website? I believe it would be best to keep this consistent with other projects under AnitaB.Org Open Source.

@mtreacy002 is it possible to have AnitaB.orgs mailing address there?

@Vuyanzi , I believe @isabelcosta has given the answer to this in her comment below which is to not use it atm. We can revisit this later when the app is being prepared for production 😉

Rahulm2310 · 2021-04-21T19:36:04Z

src/home/Home.jsx

+        <div>
        <section id="home">
        <div className="header-text">
        <h1>Welcome to<br/> Bridge-in-Tech</h1>


Suggested change

<div>

<section id="home">

<div className="header-text">

<h1>Welcome to<br/> Bridge-in-Tech</h1>

<div>

<section id="home">

<div className="header-text">

<h1>Welcome to<br/> Bridge-in-Tech</h1>

Rahulm2310 · 2021-04-21T19:37:55Z

src/home/Home.jsx

        </p>
        </div>
        </section>
+        <Footer/>
+        </div>


Suggested change

</p>

</div>

</section>

<Footer/>

</div>

</p>

</div>

</section>

<Footer/>

</div>

Rahulm2310 · 2021-04-21T19:40:30Z

@naveen8801 Have you updated the GIF in the PR description? I can't see the changes

naveen8801 · 2021-04-22T03:40:13Z

@naveen8801 Have you updated the GIF in the PR description? I can't see the changes
yes @Rahulm2310 , i have updated it now :)

naveen8801 · 2021-04-23T05:29:19Z

Hi @Rahulm2310 @mtreacy002 , i am not able to find why these tests are falling :( , Please help !

mtreacy002 · 2021-04-23T05:36:40Z

@naveen8801 , we're facing an error on the initial pr on test.yml creation (PR #170) and still trying to figure it out. You don't need to worry about why the test is failing on this PR for now, as it might not be coming from your PR. Once we troubleshoot the cause for #170 failing tests, we could rerun the tests here to see if the tests are really failing or not 😉. cc @jalajcodes and @Rahulm2310

Rahulm2310 · 2021-04-23T06:53:35Z

FAIL src/tests/Register.test.js
  ● Console

    console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29

      Error: Uncaught [Error: Invariant failed: You should not use <Link> outside a <Router>]

@mtreacy002 @naveen8801 Test cases for Register page are failing.

mtreacy002 · 2021-04-23T06:59:15Z

FAIL src/tests/Register.test.js
  ● Console

    console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29

      Error: Uncaught [Error: Invariant failed: You should not use <Link> outside a <Router>]

@mtreacy002 @naveen8801 Test cases for Register page are failing.

@Rahulm2310 , yeah, this gets me confused since the tests passed on @aditmehta9 's pr branch as I mentioned here 🤔

mtreacy002

@naveen8801 , please check my feedback below 😉.

mtreacy002 · 2021-04-24T01:17:50Z

src/home/Footer.jsx

+            </ul>
+          </div>
+          <div className="flexbox-item">
+            <h3 className="heading">Visit</h3>


@naveen8801 , I believe @Rahulm2310 has answer your question so you can remove this 😉. @Vuyanzi and @isabelcosta unless you'd prefer this Visit to show AnitaB.Org mailing address as per anitab.org website? I believe it would be best to keep this consistent with other projects under AnitaB.Org Open Source.

mtreacy002 · 2021-04-24T01:20:58Z

FAIL src/tests/Register.test.js
  ● Console

    console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29

      Error: Uncaught [Error: Invariant failed: You should not use <Link> outside a <Router>]

@mtreacy002 @naveen8801 Test cases for Register page are failing.

@Rahulm2310 , I opened issue #239 to address the failing tests. Please give your suggestion on the approach we could use.

mtreacy002 · 2021-04-24T01:34:43Z

@naveen8801 and @Rahulm2310, I'm putting the status on hold to wait for the response from @Vuyanzi and/or the @anitab-org/admins for the above question. PS: I've tested the pr locally and everything looks fine. Only need to wait whether or not to remove the Visit info from the footer and will submit the review/test report afterward.

isabelcosta · 2021-04-25T11:23:05Z

regarding this comment #228 (comment), I think for now we remove the address and focus on the app-specific content, and leave these legal / real contact information, for later.

cc @Vuyanzi @mtreacy002

mtreacy002 · 2021-04-25T11:27:01Z

regarding this comment #228 (comment), I think for now we remove the address and focus on the app-specific content, and leave these legal / real contact information, for later.

cc @Vuyanzi @mtreacy002

Thank you, @isabelcosta for the answer 🥰. @naveen8801 , can you please remove Visit from the code then. Please don't forget to resolve the conflict before you push another commit. I'll review it once you've done. 😉

naveen8801 · 2021-04-25T11:44:45Z

regarding this comment #228 (comment), I think for now we remove the address and focus on the app-specific content, and leave these legal / real contact information, for later.
cc @Vuyanzi @mtreacy002

Thank you, @isabelcosta for the answer 🥰. @naveen8801 , can you please remove Visit from the code then. Please don't forget to resolve the conflict before you push another commit. I'll review it once you've done. 😉

sure @mtreacy002 , I'll do that 👍

mtreacy002

LGTM, @naveen8801 ❤️ .
Tested this locally. Hre's the test summary.

Code review: Done
Tested all possible outcomes
Expected behaviour: Footer section is shown on the Homepage and responsive to different screen sizes.
Actual behaviour: As expected.
Environments:
OS: MacOS Catalina 10.15.7
Browser: Google Chrome

Thank you again @naveen8801 for your contribution. 🎉 🎉 🎉

Now we only need @Rahulm2310 or @Vuyanzi 's approval before we can merge this to develop branch 😉

Vuyanzi

Looks good @naveen8801 🎊 thanks for your contribution!

Rahulm2310

LGTM 🎉 💯. Thanks @naveen8801

mtreacy002 · 2021-04-28T10:12:35Z

Thanks @Vuyanzi and @Rahulm2310 for your approvals. Merging it now 😉

Footer Component Added

d41697d

isabelcosta added the Status: Needs Review PR needs an additional review or a maintainer's review. label Apr 12, 2021

isabelcosta requested a review from mtreacy002 April 12, 2021 23:48

shades-7 suggested changes Apr 13, 2021

View reviewed changes

Amulya-coder added Category: User Interface Improvements or additions to design. Open Source Hack 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 13, 2021

UI fixes and Social Link added

644c5b0

mtreacy002 requested changes Apr 17, 2021

View reviewed changes

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 17, 2021

mtreacy002 reviewed Apr 17, 2021

View reviewed changes

Warnings fixed and Footer added on Homepage

a586608

mtreacy002 requested changes Apr 17, 2021

View reviewed changes

Rahulm2310 reviewed Apr 17, 2021

View reviewed changes

naveen8801 requested a review from Rahulm2310 April 21, 2021 04:17

Rahulm2310 reviewed Apr 21, 2021

View reviewed changes

Indentation added

9c3d982

mtreacy002 reviewed Apr 24, 2021

View reviewed changes

mtreacy002 added Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Apr 24, 2021

Visit Section removed

be893e1

Rahulm2310 added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. labels Apr 26, 2021

mtreacy002 approved these changes Apr 27, 2021

View reviewed changes

Vuyanzi approved these changes Apr 27, 2021

View reviewed changes

Rahulm2310 approved these changes Apr 27, 2021

View reviewed changes

Rahulm2310 added Status: Ready to Merge Work has been tested and needs a final review and merge from a repo maintainer. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 27, 2021

mtreacy002 merged commit e50da2c into anitab-org:develop Apr 28, 2021

mtreacy002 added the Type: Enhancement New feature or request. label May 2, 2021

mtreacy002 assigned naveen8801 May 2, 2021

Footer Component Added #228

Footer Component Added #228

Conversation

naveen8801 commented Apr 12, 2021 • edited Loading

Description

Type of Change:

Checklist:

shades-7 commented Apr 12, 2021

isabelcosta commented Apr 12, 2021 • edited Loading

mtreacy002 commented Apr 13, 2021

shades-7 left a comment • edited Loading

Choose a reason for hiding this comment

naveen8801 commented Apr 13, 2021 • edited Loading

Rahulm2310 commented Apr 14, 2021

naveen8801 commented Apr 15, 2021

Rahulm2310 commented Apr 15, 2021

naveen8801 commented Apr 15, 2021

mtreacy002 left a comment

Choose a reason for hiding this comment

naveen8801 commented Apr 17, 2021

mtreacy002 commented Apr 17, 2021

naveen8801 commented Apr 17, 2021

mtreacy002 left a comment

Choose a reason for hiding this comment

naveen8801 commented Apr 17, 2021 • edited Loading

mtreacy002 left a comment • edited Loading

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 Apr 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

Choose a reason for hiding this comment

mtreacy002 Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahulm2310 commented Apr 21, 2021

naveen8801 commented Apr 22, 2021

naveen8801 commented Apr 23, 2021

mtreacy002 commented Apr 23, 2021

Rahulm2310 commented Apr 23, 2021

mtreacy002 commented Apr 23, 2021

mtreacy002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Apr 24, 2021 • edited Loading

mtreacy002 commented Apr 24, 2021 • edited Loading

isabelcosta commented Apr 25, 2021

mtreacy002 commented Apr 25, 2021

naveen8801 commented Apr 25, 2021 • edited Loading

mtreacy002 left a comment • edited Loading

Choose a reason for hiding this comment

Vuyanzi left a comment

Choose a reason for hiding this comment

Rahulm2310 left a comment

Choose a reason for hiding this comment

mtreacy002 commented Apr 28, 2021 • edited Loading

naveen8801 commented Apr 12, 2021 •

edited

Loading

isabelcosta commented Apr 12, 2021 •

edited

Loading

shades-7 left a comment •

edited

Loading

naveen8801 commented Apr 13, 2021 •

edited

Loading

naveen8801 commented Apr 17, 2021 •

edited

Loading

mtreacy002 left a comment •

edited

Loading

Rahulm2310 Apr 17, 2021 •

edited

Loading

mtreacy002 Apr 27, 2021 •

edited

Loading

mtreacy002 commented Apr 24, 2021 •

edited

Loading

mtreacy002 commented Apr 24, 2021 •

edited

Loading

naveen8801 commented Apr 25, 2021 •

edited

Loading

mtreacy002 left a comment •

edited

Loading

mtreacy002 commented Apr 28, 2021 •

edited

Loading