feat: creating a footer #42
Conversation
✅ Deploy Preview for accessibility-mentor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This reverts commit 79ad5a7.
|
Nice start so far @CBID2 🙂 |
Thanks @YuriDevAT! 😊 I just need to figure out a way to add the CSS 🤔 |
|
Hi @YuriDevAT! :) My PR is officially ready for review! :) |
Oh sorry, I missed this comment! |
|
Can you share a screenshot with the footer showing on your screen? I implemented it (since it wasn't already and there were no further instruction on how to implement it) by adding it to the But I get a compile error because the logo was not imported right. |
There was a problem hiding this comment.
Thank you so much for your contribution to the project 😃 I left some comments. After making some changes I am happy to merge your PR. You are doing such a great job 🔥
Always keep in mind to test your implementation, checking, if everything is working on your side as expected. It makes PR approvals way faster. 🚀
pages/components/Footer.js
Outdated
| const Footer = () => { | ||
| return ( | ||
| <footer style={{ width: "100%", position: "relative", bottom: "0"}}> | ||
| <Image src={Logo} alt="footer-logo" width={100} height={100}></Image> |
There was a problem hiding this comment.
Nice usage of the Image tag 😄. It can be used as a self closing tag, written as <Image src={Logo} alt="footer-logo" width={100} height={100} />, as tags for images are generally written in HTML.
pages/components/Footer.js
Outdated
| const Footer = () => { | ||
| return ( | ||
| <footer style={{ width: "100%", position: "relative", bottom: "0"}}> | ||
| <Image src={Logo} alt="footer-logo" width={100} height={100}></Image> |
There was a problem hiding this comment.
Image size is reforming the logo.
Correct implemention of default width and height directly on the component 👏🏽 If the ration is not known it can be calculated, or (which is not best practice but would do it for now), use height="auto" to keep the ratio of the logo.
pages/components/Footer.js
Outdated
|
|
||
| const Footer = () => { | ||
| return ( | ||
| <footer style={{ width: "100%", position: "relative", bottom: "0"}}> |
There was a problem hiding this comment.
Can you explain the usage of position here? I could not see a difference when not using it. Maybe I oversee something here.
There was a problem hiding this comment.
Hi @YuriDevAT. I was struggling with moving it to the bottom position: relative; So I found this tutorial to help me.
There was a problem hiding this comment.
Thanks for sharing. The tutorial is good! Is it working as expected on your side? As mentioned in the tutorial, the position attribute for the footer depends on the settings of the main component. Since you did not make any changes there I guess it will not work.
|
Hi @YuriDevAT! :) I revised the file path for the image import component, but for some reason, it does not show up in the deployment. Did I do something wrong? Also, I use |
Where did you import the footer? The logo shows up on my side with this path. |
I didn’t commit that line of yet @YuriDevAT. When I tried it locally, nothing showed up. For context, I did the import like this: |
|
Hi @YuriDevAT. I managed to get the footer to show. I also added the screenshots in the Before and After section. Do you mind if I add my code to the |
Ah, I see now where the miscommunication lies. My apologies 🙏🏽 .
was meant to implement it for everyone to see and to check it for PR, therefore it is needed to import the footer component in the |
It's working now, right? The logo with the original path is showing up in your local environment. |
pages/components/Footer.js
Outdated
| }} | ||
| > | ||
| <Image src={logo} alt="footer-logo" width={100} height="auto" /> | ||
| <p className="copyright"> |
There was a problem hiding this comment.
Is this class in use? If not, then it can be deleted.
pages/components/Footer.js
Outdated
| textAlign: "center", | ||
| }} | ||
| > | ||
| <Image src={logo} alt="footer-logo" width={100} height="auto" /> |
There was a problem hiding this comment.
Concerning accessibility, think of a better alt text for the logo, or if alt text is even needed.
There was a problem hiding this comment.
Hi @YuriDevAT! :) I think it's best to keep the alt text so it can be read by screen readers.
There was a problem hiding this comment.
You are right, alt text is for being read by screen reader users, but that does not necessarily mean you should add one. Especially when it comes to logos (which does not link to anything, yet), the alt text should be left empty like this alt="".
Read this article about alt text decision tree, it is really helpful to understand the need of alt text better https://www.w3.org/WAI/tutorials/images/decision-tree/
There was a problem hiding this comment.
A detailed alt text is typical and very good when it comes to images, I never saw it for logos, though. Logos are mostly used as links (like, Go To Homepage, or Go To Website when it is a logo which links to another companies website). But at the current state, the logo does not link anywhere, so no alt text needed (it will be a future issue, though, so keep your eyes open 👀 )
❗ But I like how detailed you are able to describe even a logo, this is a skill not many people have. Hopefully, you make good use of it in the future, when alt text is needed on descriptive images 🙌🏽
pages/components/Footer.js
Outdated
| margin: "auto", | ||
| textAlign: "center", |
There was a problem hiding this comment.
Isn't necessary to fulfill the AC 👇🏽
- No additional styling needed
Yes |
|
Hi @YuriDevAT. I forgot to mention this in my Linkedin DM. I decided to make the logo's alt text a bit more detailed. Here's what I wrote:
|
There was a problem hiding this comment.
Hey you 😄 Last time revising your PR. Small adjustments but then it should be ready to go.
Please read my comments carefully and make sure you understand everything I mentioned as well as the ACs. If not, please ask me. Less code is more in this case. You will get a feeling for it soon, you will see 😎
pages/components/Footer.js
Outdated
| display: "flex", | ||
| flexDirection: "column", | ||
| alignItems: "center", | ||
| textAlign: "center", |
There was a problem hiding this comment.
Not needed. Please stick to the AC. Also discussed on LinkedIn 👇🏽
just go without position, just leave the width:100% and we are good to go 😊
Next steps 👉🏽 Remove everything except the width. Only do as much as described in the AC to get your PRs merged. The design of the footer will look nothing alike as it is now. So, all this code is going to be removed with the next ticket for the footer already, which is unnecessary work for the next contributor as well as for the maintainer mentioning this step in the ticket.
Expected output
<footer
style={{
width: "100%",
}}
>
Wish 🙏🏽 : Please talk and explain reasoning behind your decisions for me to understand certain things better. If there are things you do not understand, ask me! This prevents unnecessary work for both of us which is very much appreciated to create a smooth work experience.
There was a problem hiding this comment.
Thanks for your input with this @YuriDevAT. I was going back and forth with using Flexbox. I wanted to show of my knowledge a bit more (plus Flexbox is easier to use in my experience), but I wanted to follow the task list. Unfortunately, I went with my slightly more ambitious side. No worries, I'll definitely stick with your suggestions this time.
There was a problem hiding this comment.
I know the feeling all to well 😁. But you will be able to show off your skills when we dive deeper into the code base, I promise 🤞🏽
pages/components/Footer.js
Outdated
| textAlign: "center", | ||
| }} | ||
| > | ||
| <Image src={logo} alt=" " width={100} height="auto" /> |
There was a problem hiding this comment.
Please remove space between quotes:
Now: alt=" "
After: alt=""
Signed-off-by: Christine Belzie <105683440+CBID2@users.noreply.github.com>
|
Hi @YuriDevAT! I read your comments carefully like you suggested and made the adjustments |
|
You did it 🥳 Thank you so much for your contribution, @CBID2 💟 I am sooo grateful for this experience. This PR taught me a lot about myself, how I have to improve my communication skills, and that there are so many things I have to consider and work on to be a good maintainer. Thank you. Couldn't have done it without you 🙏🏽 Looking forward to your next contribution, Christine 😄 |
For an example of how to fill this template out, see this Pull Request in the Wiki.
Description
This PR creates a footer.
Fixes Issue
Closes #16
Acceptance Criteria
Type of Changes
Make sure your code follows the code style of this project.
Make sure your code is up2date.
The title of the PR is the same as the title of the issue.
Updates
Before
After
Testing Steps / QA Criteria