London<Leili Heidarishizari<User-Focused-Data Module<Wireframe<Week 1 #180
Conversation
✅ Deploy Preview for cyf-ufd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
leiliheidarishizari
added
the
Needs Review
|
The most up to date PR ;) |
|
Hi lovely viewers. Your insight of my PR would be highly appreciated. Thanks in advance. |
Hi @leiliheidarishizari ! Thanks so much for this. Good start! Look over your PR: does the title match the format asked for? Have you completed all the actions in the task list? Are there questions for your reviewer? Take a look at https://github.com/CodeYourFuture/Module-User-Focused-Data/pulls to see how others are doing this. Let us know when you've updated this PR to meet those requirements. 🙏 |
SallyMcGrath
added
the
👀 Review Requirements
leiliheidarishizari
changed the title
leiliheidarishizari
changed the title
|
|
Hi Sally, thank you so much for your comments. I believe that I made a very basic mistakes which is very embarrassing. It was my first project so I was not very familiar with the requirement structures. From now on, I would do all the requirements accordingly :) |
Definitely working on requirements is important - and something we can all always improve. But oh, if you get embarrassed every time you get help and improvements in code review you'll absolutely perish under the weight of it! Code review isn't about being judged, it's about being helped. 🫶 Developers work in teams and we all help each other. Look at this - here someone is helping me by code reviewing the curriculum CodeYourFuture/curriculum#188 . 36 comments... maybe that seems a lot but if you look https://github.com/CodeYourFuture/curriculum/pulls?q=is%3Apr+is%3Aclosed+sort%3Acomments-desc there are PRs with over 100 comments! Here's people building React over on facebook. 212 comments on just one PR! ⛰️ 😉 |
|
Hi dear Sally, I've changed everything that you asked accordingly. I also checked the lighthouse which at first was low because of the large size of the images, then I resized them and checked the light house again, all of them are 100% apart from the CEO.I have to check the reasons for that as well. Thanks again for reviewing my PR. |
leiliheidarishizari
added
the
Complete
leiliheidarishizari
changed the title
cjyuan
removed
👀 Review Requirements
There was a problem hiding this comment.
Hi @leiliheidarishizari .Thanks for this! 🎉
Your work is deployed here https://deploy-preview-180--cyf-ufd.netlify.app/wireframe/
When I ran lighthouse it told me you had an error. Your touch targets are too small. Run the test and click the links to understand how to fix this. All the help is right there in Devtools. Good luck!
|
Hi dear Sally. Thanks for you comment. I couldn't find touch targets in my dev tools. I searched about it and I found out ,this error looks like this "Tap targets are not appropriately sized"!!! But I can not find this error! |
Touch targets and tap targets are the same thing. Run the test and click the links on the report -- it will show you what to do. Press the disclosure triangle to find the instructions. |
|
Hi dear Sally. Thanks for sending the image. It is so clear now however when I check the lighthouse, the accessibility show 100 .I'm wondering why you see 93 with the Touch Target issue. Please kindly find the below screenshot as a proof. Many thanks.
|
Hmm, probably the settings on our respective Lighthouses. Try changing them around -- switch from mobile to desktop, as this is a desktop layout. |
|
Hi dear Sally, I completed your request and the problem has been solved now. Woohoo ;) |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Hi Sally, do you see any potential issues with making this wireframe responsive for mobile and tablet views? and
Would you suggest any changes to improve adaptability across different screen sizes?