@javiIT10Submitted 4 months ago
SamO
@SamOwensAll comments
- @SamOwensPosted 4 months ago
Looks good to me. A couple of little things:
- The name has 4px padding below it, but its tricky to see on the designs. That should get the alignment even more perfect.
- Where you've used
text-[14px]you could have just usedtext-smas its already built into tailwind. There may be a few other instances of this
Really good job though, the code is very well organised.
0 - @shinzenkoSubmitted 4 months agoWhat are you most proud of, and what would you do differently next time?
Being able to put the layout down and execute it properly, I would like to improve upon my time that i take to make such app.
What challenges did you encounter, and how did you overcome them?challenges weren't that blocking for me i could pass by most of them without much help.
@SamOwensPosted 4 months agoLooks pretty good. Just a few very minor things
- If possible you should use rem values for sizes as opposed to px.
- You don't really need the surround div, so it would be considered DOM bloat.
- You'd be better off giving the images their own class rather than using their wrapper to target them but again, very very minor.
- The padding over the SVG looks a bit off
1 - @Dreez-WebSubmitted 4 months ago@SamOwensPosted 4 months ago
You’re referencing the style sheet wrong, so no css is pulling through to the HTML file.
Changing it to style.css rather than /style.css should fix it
Marked as helpful0 - @saad-muhammad01Submitted 4 months agoWhat are you most proud of, and what would you do differently next time?
Being able to increase confidence in development.
What challenges did you encounter, and how did you overcome them?Trying to center elements, I looked up online and found helpful solutions
What specific areas of your project would you like help with?Having to Design the UI, I can replicate them through code by trying to come up with a design is one important thing.
@SamOwensPosted 4 months agoNot bad. A few things I noticed:
-
You should set a fixed width and height to make sure the card is the same size as the design. (This is only true if the component is small enough that it won't overflow the viewport on mobile)
-
The padding seems to be a little bit off between elements.
-
The QR code image should have a smaller border-radius than the card. This can be done directly on the img tag using
overflow: hidden;
Marked as helpful1 -
- @RajKumar-612Submitted 4 months agoWhat are you most proud of, and what would you do differently next time?
some how completed, would like to complete it faster.
What challenges did you encounter, and how did you overcome them?took time for styling, gone through tutorials online and some solutions.
What specific areas of your project would you like help with?Can anyone review the css and provide feedback on what could be done better.
@SamOwensPosted 4 months agoLooks good.
Ideally I think you should be using rem rather than px, but given there's few 1:1 conversion for the sizes used on the design its probably fine.
Marked as helpful0 - @CelineJamesSubmitted 4 months agoWhat are you most proud of, and what would you do differently next time?
i am getting better with using CSS, i am proud of that.
What challenges did you encounter, and how did you overcome them?i had the challenge of designing the links, could not quite decide if i should use buttons or just links, i used buttons and then wrapped the links inside the buttons however i had difficulty styling it. so i had to use just the links and style them like buttons.
@SamOwensPosted 4 months agoLooks good!
Other feedback has most points covered, but the one I would add is that you should include
aria-labelattributes to your links for better accessibility. You can read more about them here - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelMarked as helpful1
