-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix for issue #86 - MUI back to top btn added to project #90
Conversation
@techy1999 please review this PR, thanks |
import { Box, Grid, IconButton } from "@mui/material"; | ||
import Typography from "@mui/material/Typography"; | ||
import Paper from "@mui/material/Paper"; | ||
import { FOOTER_MENUS } from '../constants/footer/footer.constants' | ||
import { FOOTER_MENUS } from "../constants/footer/footer.constants"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed change for your PR. Why you are making it.
import YouTubeIcon from "@mui/icons-material/YouTube"; | ||
import FacebookIcon from "@mui/icons-material/Facebook"; | ||
import InstagramIcon from "@mui/icons-material/Instagram"; | ||
import LinkedInIcon from "@mui/icons-material/LinkedIn"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue
<Grid | ||
container | ||
spacing={{ xs: 2, md: 4 }} | ||
columns={{ xs: 4, sm: 8, md: 12 }} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue
<a | ||
style={{ cursor: "pointer", color: "white" }} | ||
href={footer_item.URL} | ||
> | ||
{" "} | ||
{footer_item.TEXT}{" "} | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue
<Box | ||
style={{ | ||
marginTop: "30px", | ||
display: "flex", | ||
alignItems: "center", | ||
justifyContent: "center", | ||
flexDirection: "column", | ||
}} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue
<IconButton color="white"> | ||
<FacebookIcon /> | ||
</IconButton> | ||
</Box> | ||
<Typography variant="caption" color="white" textAlign={"center"}> | ||
Copyright ©{new Date().getFullYear()}. NOMADS.SOLUTIONS | ||
Copyright ©{new Date().getFullYear()}. NOMADS.SOLUTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now i am approving this PR and merging it.
Suggestion
- check the changes you have made are correct or not before creating any PR. Do not make any unnecessary changes in codebase apart from the feature/bug you are working on. @Sky-De
- Apart from this you PR looks good
Thank you for contributing 💗 , . If you need any help go to discussion Tab and put your question there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@techy1999 thanks, but all those changes happened because of prettier, and i mentioned that, also according to issue #88 you will need all those changes on all code source later,
But for now it will be ok, don't worry it
This PR addresses issue #86
Changes
Fab and NavigationIcon imported from MUI
Added functionality for scrolling to the top as a handler named "backToTopHandler" implemented in the footer component with a new style to be positioned at the top of the footer.
Screenshots
! ! ! ! ISSUE FOUND
I found an issue with mobile responsiveness. Attached is a GIF showing the problem. This issue existed before I added the back-to-top button. You can open an issue related to this and assign that to me, then i will fix it
Consideration
if you have any questions feel free to ask, otherwise let me know you are happy with this PR by merging it,
thanks
skyDe