Skip to content
OverviewSolutions
9
Comments
10
Omar M.
@OmarMAttia7

All comments

  • Omar M.•270
    @OmarMAttia7Posted about 2 years ago

    Hello, congratulations on finishing the challenge and submitting your solution.

    • For starters, like @SHMITEnZ said the <picture> element is a good solution for responsive images in HTML, to learn more about it in depth I suggest you read this guide by MDN.
    • You should wrap all sections of your document in landmarks to improve accessibility, in this case, you should wrap your card component in a <main> element since it is the only thing on the page, however if you have a bigger document with more content you would probably wrap it in a <section> or <article> or it would be in a bigger component wrapped in one of these.
    • Lastly your .card class has an overflow: hidden attribute, this will prevent people from scrolling on shorter screens, try decreasing your browser window's vertical size to see for yourself. you should remove it, I'm curious as to why you added it?

    Otherwise you did a fantastic job, good luck and keep coding :).

    Marked as helpful
    1
  • Abrosss•440
    @AbrosssSubmitted over 2 years ago

    Hello. I am not sure if height and width of the container should be dynamic, depending on the random advice text or fixed?

    Random advice API

    2
    Omar M.•270
    @OmarMAttia7Posted over 2 years ago

    Hey, good job you did there 👍.

    Like @byronbyron said your question is not technical it's more concerned with design.

    But in case you want to make it a fixed width you should go with a percentage for smaller screens and a reasonable max width for larger ones. If your component is a flex item use vw instead of percentages.

    Marked as helpful
    1
  • Kamrul Saad•90
    @kamrulsaadSubmitted over 2 years ago

    Can anyone suggest me how I can reduce the classes for buttons in the code?? for setting colour to the texts to be like that of the card I had to do in individually. Is there any easier way to do that?

    3-column-preview-card component

    2
    Omar M.•270
    @OmarMAttia7Posted over 2 years ago

    Good job finishing the challenge 🎉✨.

    Unfortunately there isn't a way to assign the color values automatically unless you're working from JavaScript or using a CSS preprocessor, you can however, make your code more manageable by naming the classes more clearly, for example like this .btn-orange, .btn-cyan and .btn-darkcyan instead of just .btn-(1,2,3...).

    Marked as helpful
    0
  • Daniel•140
    @obriedanSubmitted over 2 years ago

    I would really love to figure out how to make this responsive without using media queries, because as it is the card works at 1440px and 375px with some super janky results in between and lower than 375.

    The margin between each section from Order Summary down changes at different rates, and the font sizes change so subtly I'm not sure how to create the exact dimensions of the mobile version using things like min(), max(), or clamp.

    Using Media Queries

    3
    Omar M.•270
    @OmarMAttia7Posted over 2 years ago

    Congratulations on finishing the challenge 🎉. Good job!

    I'm not entirely sure I got your question right, what exactly do you dislike about the current design? I'm not sure I see a problem here but I'm new to responsive design as well. You shouldn't be concerned with devices smaller than 320px in width, in the rare case someone uses a device like that they could just scroll horizontally.

    I also noticed that you're using exact measurements with pixels, in the case you want to change measurements on different screen widths you should use rems as reference for your measurements and then just changing the font-size of the html element at different media queries and adjusting the font-size of text elements if they get too small or big you could look here for more information on how rem works.

    Also you should turn height: 100vh to min-height: 100vh in your container because it breaks your design on landscape orientation and other small heights.

    Try looking into working with a mobile-first approach to responsive design it could make things clearer for you.

    Marked as helpful
    1
  • Elysson Tanaka•90
    @elyssontanakaSubmitted over 2 years ago

    Hi guys!! This is my first challenge, I would appreciate any comments or suggestions to help improve the quality of my code or on any mistakes you find, especially on the responsiveness part.

    Thanks!

    NFT Preview Card Component using flexbox

    1
    Omar M.•270
    @OmarMAttia7Posted over 2 years ago

    Congratulations on finishing the challenge 🎉. Good job! I have some feedback on your solution.

    First I would like to point out that your usage of the <section> element is a little confusing, you should use a <div> for styling purposes. Check this page for information on how to use it correctly.

    Second thing I noticed is that you're way too focused on getting the design pixel perfect, you should be more focused on things like layout and accessibility, how would your component work when put in the same space with other similar components?, will it work correctly if put in another document with other content?, will it resize correctly if a visually impaired user increased the font size?, If someone is using a screen reader will they be able to navigate it correctly?.

    By focusing too much on getting the design perfectly you are missing on other important things.

    I suggest using flex to center things on the page instead of position: absolute and using ems instead of pixels for starters.

    As for responsiveness, you worked on the desktop and large screens first and then made appropriate media queries for mobile and other smaller screens, that works fine most of the time. but you usually want to work with a mobile-first workflow where you make sure your code is working correctly on smaller screens and then expanding from there as it gives you more freedom and takes much less effort, check this article for more information on the subject.

    Marked as helpful
    1
  • Omar M.•270
    @OmarMAttia7Posted over 2 years ago

    Congratulations on finishing the challenge, good job that was solid :).

    I have a couple of things to point out:

    1. You should wrap the main content of your website in a <main> element or something equivalent.
    2. You should also get your font-size a little bigger, most people will have trouble reading text this small. In a real-life situation your designer is probably gonna provide you with accurate font sizes but it's good to know anyway.

    Good luck and keep coding <3.

    Marked as helpful
    1
  • Omar M.•270
    @OmarMAttia7Posted almost 3 years ago

    Congratulations on finishing the challenge :). Your is code has some problems with HTML Markup and CSS:

    1. The main content of your document should be wrapped inside a <main> element or something equivalent, in this case it's preferable that you give it class="container" instead and delete the <div class="container"> you have as it has no use
    2. The alt attribute of your images should have a meaningful description of the image for people who can't view it visually, you shouldn't leave them empty.
    3. All HTML documents should have an <h1> element, in this case your Order Summary should be an <h1> instead of an <h3>, you can change the font-size with CSS.
    4. The Proceed to payment and Cancel Order are user interactions so they should be appropriate semantic elements like <button>s or <a>s to avoid accessibility issues.
    5. You should use percentages and ems whenever possible instead of pixels because some users will scale the website to their own preference and you want it to look OK for them.
    6. The hero image should usually be added using CSS backgrounds instead of HTML <img> as it is there for decoration.
    7. Finally your CSS could use some organization, consider commenting your css files and using a naming methodology like the BEM method. Good luck and keep coding <3.
    1
  • RonanC•30
    @RonanCaSubmitted almost 3 years ago

    Hey ! I'm open to any suggestion, I struggled a little bit with the hover effect on the main image.

    NFT preview card component

    1
    Omar M.•270
    @OmarMAttia7Posted almost 3 years ago

    Congratulations on finishing the challenge :). You have some problems with HTML markup, most of them are in the report:

    1. All images must have a meaningful alt attribute that describes the image for people who can't view the image visually.
    2. The main content of your page should be inside a <main> element, it's preferable that you give it the class="container" instead and remove the <div class="container"> as it has no use.
    3. [the footer element should be inside the body element] (https://teamtreehouse.com/community/why-is-the-footer-placed-before-the-closing-of-the-body-tag-and-not-after).
    4. Any HTML document should have an <h1> heading element, in your case you should put <a class='title'>Equilibrium #3429</a> inside an <h1>.

    Good luck and keep coding <3.

    Marked as helpful
    1
  • Omar M.•270
    @OmarMAttia7Posted almost 3 years ago

    Congratulations on finishing the challenge :). You have a few problems with standard html5 semantic markup, most of them are in the report of your solution.

    1. All your <img> elements should have an alt attribute that describes the image for people who can't view the images like so <img src="images/image-avatar.png" alt="Jules Wyvern">.
    2. The main content of your page should be contained within a <main> element or something equivalent, like so<main> <div class="card-main">
    3. Your page should have one <h1> element, in this case it should be <h1 class="nft-name"> Equilibrium #3429</h1> you can adjust the font-size using css.
    1
  • Andy Ouyang•30
    @ahouyangSubmitted almost 3 years ago

    Any general tips on code styling/best practices are always appreciated. I also used flexbox basically any time I needed to move a component around, wasn't sure if that's always ideal or not. Thanks!

    Order summary challenge

    2
    Omar M.•270
    @OmarMAttia7Posted almost 3 years ago

    Congratulations on finishing the challenge, Good job :).

    I'd like to point out a couple of problems with your HTML markup:

    1. Your component should be wrapped inside a <main> element. Every HTML document should have a <main> element.
    2. The title of your document - in this case Order Summary - should be put inside an <h1> element, also every HTML document should have one.
    Marked as helpful
    0