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

Issue #49 Resolved (Footer) #51

Merged
merged 2 commits into from
Jun 23, 2022
Merged

Conversation

Jai-Solania-29
Copy link
Contributor

Fixes Issue

Closes #49

Changes proposed

Social Icons in the footer are now accurate and according to the design of the website.
Redirection links are given to the social icons, and they all are in working state following the target blank property.
Footer Size is modified according to the official design of the website.
Alignment of text and sponsor card fixed.
Sponsor Card size is modified according to the official design of the website.
Design of Sponsor Card is also modified and its now accurate and according to the design o the website.
Font Size is modified according to the design of the website.

Screenshots

Previous Footer:

Screenshot (65)

Current Footer :

Screenshot (68)

Note to reviewers

cc- @AvineshTripathi @verma-kunal @SuperAayush

Previously SVG format images were used for social icons, but they were not working properly as for all social icons same and only one image was rendering which was in the front line of the code.
So for fixing that issue I have now used React Icons instead of SVG files.

Copy link
Collaborator

@AvineshTripathi AvineshTripathi left a comment

Choose a reason for hiding this comment

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

After trying the changes I see the overall changes alignment of the footer component is disturbed on resizing the screen please have a look into it

Also we need some space between the mail logo and the mail Id

@@ -6,6 +6,15 @@
*/
import React from 'react';
import styles from './styles.module.css'; // CSS modules
import { BsInstagram } from "react-icons/bs";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this import used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I have corrected this also.

<a target="_blank" href="https://www.linkedin.com/company/kubesimplify/"><TiSocialLinkedin row="img" className={styles.social2}/></a>
<a target="_blank" href="https://www.instagram.com/saiyampathak/"><AiOutlineInstagram row="img" className={styles.social2}/></a>
<a target="_blank" href="https://github.com/kubesimplify"><AiFillGithub row="img" className={styles.social3}/></a>
{/* <a href="#"><logo.instagram.Svg row="img" className={styles.social} /></a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we commenting these line and not removing these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed these lines now, and they will reflect in my next commit.

@Jai-Solania-29
Copy link
Contributor Author

Jai-Solania-29 commented Jun 1, 2022

After trying the changes I see the overall changes alignment of the footer component is disturbed on resizing the screen please have a look into it

Also we need some space between the mail logo and the mail Id

Actually @AvineshTripathi I didn’t work on the responsive thing because it was already not responsive initially, But ok I’ll look into it and will push my changes after doing the work that you suggested.

@AvineshTripathi
Copy link
Collaborator

Hey @Jai-Solania-29 any updates

@Jai-Solania-29
Copy link
Contributor Author

Hey @AvineshTripathi actually I'm having my exams these days so I'm little busy right now. I was thinking may be you can merge this PR for now, and I'll open a different issue for responsiveness of the footer so that someone else can work on that if interested, Otherwise I'll start working on this issue in next couple of days.

@verma-kunal
Copy link
Collaborator

Hey @AvineshTripathi actually I'm having my exams these days so I'm little busy right now. I was thinking may be you can merge this PR for now, and I'll open a different issue for responsiveness of the footer so that someone else can work on that if interested, Otherwise I'll start working on this issue in next couple of days.

Sounds good to me!
@AvineshTripathi let me know your thoughts here

@AvineshTripathi
Copy link
Collaborator

All the best @Jai-Solania-29 for exams and thanks for notifying

LGTM

P.S. i ll create an issue for it after merge

@Jai-Solania-29
Copy link
Contributor Author

Thank you @AvineshTripathi , I have my last exam tomorrow. So once you create an issue I’ll start working on that.

@Jai-Solania-29
Copy link
Contributor Author

Hey @AvineshTripathi any updates ?

@AvineshTripathi
Copy link
Collaborator

Hey @Jai-Solania-29 sorry for the delay, can you create issue and start working or i should create issue?

@Jai-Solania-29
Copy link
Contributor Author

Hey @AvineshTripathi I’ll create an issue and will start working on that , but that is only possible when my PR gets merged. I mean the actual footer will reflect after my PR gets merged, and only then I can open an issue for making the footer responsive, and will start working on that .

@AvineshTripathi
Copy link
Collaborator

AvineshTripathi commented Jun 18, 2022

can you make the positioning relative and not absolute
when fixed we can merge
LGTM with a reminder to myself the package.lock.json file has updated, in case tree gets disturbed after merge

cc @verma-kunal

@verma-kunal
Copy link
Collaborator

can you make the positioning relative and not absolute

I believe this change hasn't been done yet! @Jai-Solania-29 can you pls check this one :)

@Jai-Solania-29
Copy link
Contributor Author

can you make the positioning relative and not absolute

I believe this change hasn't been done yet! @Jai-Solania-29 can you pls check this one :)

Hey @verma-kunal I have pushed the requested changes, Kindly check and merge.

__
sema-logo  Summary: 🛠️ This code needs a fix

Copy link
Collaborator

@verma-kunal verma-kunal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@verma-kunal
Copy link
Collaborator

Thanks for contributing @Jai-Solania-29

@verma-kunal verma-kunal merged commit 1d3f3ac into kubesimplify:main Jun 23, 2022
Shivansh-yadav13 pushed a commit to Shivansh-yadav13/kubesimplify-website that referenced this pull request Jul 9, 2022
* Issue kubesimplify#49 Resolved (Footer)

* Position Changed from Absolute to Relative
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.

[Bug] <Footer Social Icons are all same and not redirecting>
3 participants