Udochukwu Amaefule
@UDsGitHubAll comments
- @SierLor@UDsGitHub
Take a look at my recent solution for this. Ignore the semantic issues, I will correct them soon enough. Some things I would like to comment on about your solution include:
- I assume you didnt get the figma design file just like me, but your spacing seems waay off. If you are using windows system, try downloading this app called PowerToys. It is useful for color picking, pixel measurements and more. I used it to get measurements for somethings in the design pictures and got a rough idea on what the sizes of things were.
- Might want to have transition duration on your interactive hover elements.
- Using an image for the svgs was the right call, but you dont exactly want to be specifying width and heights on the images, cuz that locks you into positions you dont want to be in, especially with this design. Setting a height was fine, but you would rather have 100% width on the svg images. For mine, I had them in divs, and the images set to object fit contain and let flexbox handle the sizing of the divs.
Hope this helps
Marked as helpful - @gokhandemr@UDsGitHub
Very nice work here, the only thing I would recommend, is have more transitions on your hover state elements
- @kdumagalhaes@UDsGitHub
Hey Kadu, just saw your solution and it looks great. The one issue I found, was that it seems as though your main profile component isnt in the same container as the time tracking components as such, when the screen width changes below 1100px, it starts to break and look off.
Good luck with that, Id suggest having both the profile and the.... dont mind me. I just looked at the divs in your code and noticed I have said a bunch of stuff right now. Yeah so basically, your profile needs to be able to stretch with the rest of the components and look decent while at it.
Good luck 👍
- @akki251@UDsGitHub
Hey man I just saw your solution to this challenge, and it looks great. the only thing I had to pick on, was the navmenu's responsive styling. When in mobile display size and the navmenu is opened, if the screen width changes back to desktop size, the navmenu as well as the hamburger menu remains open and visible. This is a slight bug you have to fix. Good luck 👍
- @CodeSenpai101@UDsGitHub
Hey CodeSenpai, I saw your solution and it looks great. I really love the way you did the border of the card. The issues I noticed so far are:
- Your hover trigger is over the whole card instead of just the "Etherium" image.
- You could also add
cursor: pointer
to your hoverable elements that arent like that by default e.g, anchor tags. - You could add some vertical padding to your body so the attribution part isn't touching the bottom of the page. These are all I have to pick on at the moment, but you can start by taking a look at the report Frontend mentor gave you.
Good luck 👍
Marked as helpful - @AchrefFast@UDsGitHub
Hey man I really love your solution, and I have been spending the past few hours trying to study and learn from it. I am still hung up on how you made the number input appear without the increment, decrement stuff... I saw your code and it looks like you used appearance, but testing stuff out on codepen, it doesnt seem to work the same for me. Any advice?
- @MuhammadM1998@UDsGitHub
Hi Muhammad, I just saw your site and it looks great. The only things I think you should add or change, are:
- The top padding of your site. Your page seems too close to the top of the screen.
- The middle two cards are not the same width as the outer two in the larger screen sizes. I think this might be due to the issue you were having with flexbox. I just noticed and felt inclined to point it out. Good luck! 👍
- @alisbai@UDsGitHub
Hi, Ali, I just saw your site and it looks great. I also noticed you used height to reveal your FAQ questions. To that end, one way you can improve your design right now is to add a transition effect to the opening and closing of the FAQs. That would improve the user experience in my opinion.
- @xxxcrisxxx@UDsGitHub
Hi XCRISX,
I just saw your site and it looks amazing. The only things I might pick on, are the fact that you don't have any hover effects on your hoverable elements. Also, your newsletter subscription's input does not have padding for the text, so that makes it a bit uncomfortable.
Good luck! 👍
- @gelo29@UDsGitHub
Hi Gelo, Your site looks good. Here are a few things you could do to improve your work.
- Instead of selecting all possible html elements in the page, you could use the " * " selector so select all the elements like so
*{ margin: 0; padding: 0; border: 0; font-size: 100%; font: inherit; vertical-align: baseline; }
- To improve semantics and accessibility, you could wrap your main card content in the main element and give it the class or id of main like you already have on that div.
- You should have a
background-size: cover
on your body's background. - I noticed you have a lot of multiple selections on elements you wanted to style like for example "article, aside, details, figcaption, figure, footer, header, hgroup, menu, nav, section { display: block; }" You want to try and avoid long selections like that because it reduces the readability of your code. other than that, the rest of your code looks pretty good, only other thing I'd pick on is maybe adding some transition duration for your hover effects. Good luck! 👍
Marked as helpful - Instead of selecting all possible html elements in the page, you could use the " * " selector so select all the elements like so
- @GJCIII@UDsGitHub
Hi GJCIII, Your site looks great and is responsive on varying screen sizes. One thing you could do though to make it a bit better would be to fix the scale on regular (large screens aka my type of screen, aka 1024 x 720px screens) because I noticed that it was a bit too large for that size. Also, you could add a bit of color change for the buttons to make the users aware that they are hovering over said button or interacting with it, also you have an underline on your "Proceed to Payment" button. Last two things, try to center your card a bit more. I assume it was pretty difficult because you have both the card and your attribution in the same flex container, but you could always remove the attribution from the page or just remove it from the container and give it some margin to stay at the bottom of the screen. Lastly, to improve semantics and avoid accessibility issues, you should replace the div you have with a class of "card" with a main element with the same class. This would improve the HTML structure of your page and help with navigation via external tools. Good luck! 👍
Marked as helpful - @outerpreneur@UDsGitHub
A quick tip for the overlay, is to just use a
filter
style on the image and tint the color with the purple hue. Good luck 👍Marked as helpful - @aldhairescobar@UDsGitHub
Hi Aldhair, I just saw your site and it looks great. My only comments are on your html report and stylistic stuff that i think would just make the site look a tad better in my opinion... just my opinion.
- You could use either swap the h2 and h3 between your "built for modern use" and your "Smarter meetings, all in one place" or use a span or a p element for your "built for modern use" and use css it scale it appropriately.
- You could also have both the section of images and the section after that in one section because it looks like it could or should be one. That would also help with the semantics of having a heading in each section.
- I feel as though this site is slick and cool enough to have a cooler scroll bar because my first impression on the site was that that scroll bar is blocking a bit of the images in the header section and if it were slimmer and slicker, it would look a lot better. Lastly, i just noticed by mistake, that you have some cool scaling happening on focus on your links/buttons. I think if you had that scaling on the buttons/links on hover, that would be cool to, but just a personal opinion. Great site and good luck 👍
Marked as helpful - @LonelyBuddy@UDsGitHub
Hi Henry, I just saw your site and it looks great. My only comments would be on the first keyboard images pseudo element. I dont know if its supposed to be there or its more of a design element from the background because I have not attempted this challenge, but yeah I think that is what is causing most of your issues with responsiveness on smaller screensizes. I think just like Aakash has said above, you should add overflow hidden to your container element, but also make those pseudo elements absolutely positioned so that they dont affect the flow of other elements. I just noticed you already have them on absolute positioning, but yeah. Start first with the overflowhidden and work your way to figuring out the content spill on the smaller screens. Good luck 👍
Marked as helpful - @Mehul9063@UDsGitHub
Hi Mehul, Your solution looks great! one quick tip though would be to add
cursor: pointer
to all your hoverable elements to make it more noticeable that these are interactable 👍. Just went through your report and noticed you have a lot of errors basically saying you need to add alt text into your images. I know sometimes we (me) assume we dont need alt text on some images that we believe dont contain any descriptive text, but we need alt text also for accessibility by for example those who use screen readers to access our sites. Also one of your html reports said you should enclose your content in landmarks: quick tip, try to use the html elements more often that divs that would also help with accessibilty by other software that use html structure to navigate through a site 👍. Before I leave ill give an example of the html stuff. you can use amain
element and add the class ofcard
you already have to it👍.Marked as helpful - @vBlanyer@UDsGitHub
Hey Blanyer, I noticed you didnt ask for feeback this time, but I saw your site and felt inclined to comment.. it seems like your images are stretching and squishing... a quick fix to that would be to add something like so to your images
img{ object-fit: cover; object-position: center; }
that would make the image stretch to cover its div assuming it is in one, and the object position would position the image similar to background-position... so play around with that to find what works.
Marked as helpful - @amakaogujiofor@UDsGitHub
Hi Amaka, Your site looks good, there are just a few things you can do to make it work better...
- You should have a
min-height: 100vh;
on your body so it always stretches to fill the entire view height. That would help with aligning the card vertically. - Your card should have a max width... but then again that could make it small on larger screens so dont make the mistake I made of having a max width based on what i could judge from my small screensize.. it seems like the 90% you have now works pretty well, but to make it better, you could use a width of
max-width: max(90%, 25rem)
-- This would basically choose the bigger width between 90% and 25rem, so on larger screens, it would be 90%, but on smaller large screens, it would be 25rem... assuming 25rem is bigger than 90% of the body's width just note the 25rem i put there is arbitrary.. you should put a value there based on what looks like it works for you.
Aside from all my rambling, you are good! 👍
Marked as helpful - You should have a
- @vBlanyer@UDsGitHub
Hi Blanyer,
Your site looks very nice, there are just a few things that stand out to me at the moment.
- Your a tags don't have
cursor: pointer;
, - Your Breakpoint for the media query is too large... maybe reduce it to about 700px or 550px.
- You have a blurred box shadow, unlike the design.
- Lastly, your javascript is not functioning properly, so you might need to fix that.
Other than that, you have it all good 👍
Marked as helpful - Your a tags don't have