@n3kk3llSubmitted over 2 years ago
Simple static card layout. But the question I have to ask is: In some web resources I found out, that it is better for images like this QR code to display it as a background for its wrapper. Is it right?
Roberto Yukio Miyamoto Arita
@RyukioMiyamotoAll comments
- @RyukioMiyamotoPosted over 2 years ago
Impecable work Andrii! I would just adjust the color on the <p> that's differing from the proposed design.
As for the image, what I follow when solving the challenges is considering if the image would be something static or dynamic in a real project, maybe coming from the backend. For hero sections for example, I'd use the background solution as it won't likely change or have variations.
For this QR Code challenge, in my conception, there could be another card with a different QR Code somewhere down the line, so I chose to make it a <img>.
This is my personal take on this by the way, I have no idea if it's a common/good practice, I'll have to look it up later.Keep it up!
Marked as helpful1 - @KYGO5000Submitted over 2 years ago
I struggled with changing the color of <P> insights</p>
@RyukioMiyamotoPosted over 2 years ago1 - @inkfrombloodSubmitted over 2 years ago
THINGS I'M NOT SURE OF:
Hero Image border radius - I had to apply the border radius to both the DIV and the IMG. Is there a way to do this just once?
There is also 3.600 white line at the bottom - unsure of how it got there...setting display: block; removed it. But I just dont' know why that happens. (similar issue in the NFT card challenge)
For the 'annual plan' and 'price' text, I want them so the top of ANNUAL PLAN is perfectly aligned to the top edge of the music icon, and the bototm of the price is perfectly aligned to the the bottom edge of the music icon. Right now I could only figure out how to fudge with adding a bottom margin to the h2 - but that feels like a sloppy way of accomplishing this.
Still having trouble matching box shadow styles.
Also, I think the design elements for this challenge are off. The colors provided did not match the design images (see the hover state of the payment button) and the font definitely appears to be different than the one it says to use.
@RyukioMiyamotoPosted over 2 years agoHi Dan, great job. I looked into the why of display: block after my last comment and it seems the reason is that block elements take up all width available, while inline elements take only as much as they need to, which might make some whitespace visible.
For the border-radius, my suggestion would be to give it to the container rather than individually (altough in some situations this solution may be better), and while that initially won't do the trick, adding overflown: hidden will solve it.
Hope you find that helpful, have a great week!
Marked as helpful1 - @n3kk3llSubmitted over 2 years ago@RyukioMiyamotoPosted over 2 years ago
Andrii you did a great job with the design! However I find it very hard to give feedback on the minified code. I'd like to point a few things anyway:
- The rating are not navigable through keyboard, which is an accessibility issue. An improvement on this would be to use <button> or <input> instead of <div>
- Your script is not preventing the user to proceed without a rating, which gives an "You selected a undefined out of 5" on submit
- Mobile ( below width +-400px ) is not applying the style to the selected rating, although it's still functional
Hope I could be of help! Keep it up!
Marked as helpful1 - @soitirakisSubmitted over 2 years ago
Hello everyone. I tried to do this challenge as best as possible. I have a problem with the hover effect over the nft image. I do not know why the hover is bigger than the imagine. Imagine is 327331, but the hover is 327333. Why? :) If anyone can check the code quality, what can be improved? Can you check the correctness of the CSS file? T here is any recommended structure or order to write from top to bottom the CSS ? Any other feedback is highly appreciated. Thank you
@RyukioMiyamotoPosted over 2 years agoHey Andrei, good job! To fix the hover overflowing, set the img to display: block to get rid of the whitespace.
Marked as helpful0 - @inkfrombloodSubmitted over 2 years ago
By far the most difficult part was creating the image overlay. There were so many different ways to approach this and many seemed needlessly complicated, or that they required more code than seemed reasonable. I worked towards finding this simplest solution. I was able to achieve close to the desired result.
HOWEVER, the one part I cannot figure out is why my overlay is is JUST slightly larger in height than than the image itself. I currently have both the .overlay height and width set to 100% - as I believe thats what the code should be in the cleanest stay. But if I changed the height to 98% then it fits fine and looks the best. But I wanted to leave it as is in the inaccurate state, because I can't figure out why its doing that and changing it to that 98% feels like I'm just covering up a mistake that I don't understand.
One thing I learned, was that rather than just going into code to BUILD UP, I really should be BREAKDING DOWN the design first and doing some sort of wireframing or outlining first. This will help me to develop better organization and better hierarchies for when placing and labeling divs within divs.
Really looking forward to feedback on this, because this one STARTED better than the QR code, but quickly took me quite a while to get to what I wanted.
@RyukioMiyamotoPosted over 2 years agoExcellent work Dan! I had the same issue regarding the img height, while I solved it differently, there's a simple solution that is to add display: block to it, erasing the white space. I'm not 100% sure about why but I think the reason it works is because it makes the element take up all available space in its "line". Anyway, good job!
Marked as helpful1 - @florianstancioiuSubmitted over 2 years ago@RyukioMiyamotoPosted over 2 years ago
I would just recommend you change the rating from divs to buttons or inputs, as they're not currently selectable, which makes them inaccessible to keyboard users. There's also some inaccuracy regarding the design, like the circle sizes and border-radius on desktop, but nonetheless, excellent work!
1 - @lukaasz555Submitted over 2 years ago@RyukioMiyamotoPosted over 2 years ago
Great job lukasz! I would just like to make two suggestions:
- Final result is not matching 100% the proposed design, like font-size on the heading.
- Although it was a very nice addition to prevent user from submiting without a rating, there's more elegant ways to do so than using alert(), like adding an <p> that only shows on submit, for example.
There's also the matter of accessibility issues, which frontend mentor is pointing in your report:
- Document should have one main landmark
- All page content should be contained by landmarks
Basically you can solve both by just changing your wrapper div to a <main> tag, which is of significant importance to screen readers. Hope you find this helpful! Good coding.
Marked as helpful1 - @BenBoubakerMajdiSubmitted over 2 years ago
if you want to see the mobile version just click on "Preview Site" and change the browser's screen size!
@RyukioMiyamotoPosted over 2 years agoGreat job overall Majdi! Regarding the overlay, try using mix-blend-mode: multiply on a separate <div> instead of a gradient, it worked for me (even though I couldn't get the exact tone as well). Hope I could be of help, if I wasn't clear enough take a look at my solution. Also, the mobile version is missing, are you still working on it?
Good coding!
Marked as helpful1 - @Bayoumi-devSubmitted over 2 years ago
I've added
Animated Counterfor stats in this challenge@RyukioMiyamotoPosted over 2 years agoThat was a great addition, good job. I would just suggest you consider the prefers-reduced-motion query, regarding the animations.
Marked as helpful1 - @RowenTeySubmitted over 2 years ago
Used previous knowledge on the NFT project for the overlay effect! Feel free to leave any feedback, cheers 🥂
@RyukioMiyamotoPosted over 2 years agoGood job Rowen, I would just suggest adding "object-fit: cover" to the <img>, to avoid deforming it. Also, in the mobile you forgot to adjust the individual border-radius, so it's not matching the design. My suggestion is to add a border-radius to the container itself rather than the img.
1 - @FluffyKasSubmitted almost 3 years ago
Hey guys,
This was a really fun little project. Without proper data behind it, it seem like a useless component so I just focused on having fun with it.
I added a tiny animation for the data bubble so the whole thing doesn't look so static.
I also tried declaring my custom properties differently this time following Kevin Powell's video so I can manipulate colours more easily. This is definitely something I add to future projects as well!
I used fixed widths for this component. I usually go with max-width, clamp, minmax, etc for setting the width but I thought for this challenge having a fixed width just makes more sense. I tried to pay attention to having the right breakpoints, so it's still responsive, hopefully :)
For the functions (like upload, etc) I used
buttons but there may be a better solution. My logic was that these icons would add some functionality instead of pointing to different websites, sobuttonseemed the most appropriate.Since the display bar of that data usage isn't functional, I just had fun with CSS and made it purely decorative. ^^
If there was any way to improve on this, I'd love to hear it!
Have a good evening everyone!
@RyukioMiyamotoPosted over 2 years agoThis is stellar work! I've got nothing to say regarding the styling since you clearly got that covered, but I do have a suggestion regarding accessibility, as some people get motion sickness from animations, and someone once pointed out to me the prefers-reduced-motion query, which I find very important and always try to implement ever since (sorry if you're already aware of it).
Marked as helpful1
