Skip to content
OverviewSolutions
34
Comments
25
Ahmed El_Bald
@Ahmed-Elbald

All comments

  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Alright,

    I just want you to know one thing: Before you try to implement the design, consider the markup. Before you write your HTML, think about what each element represents.

    In my opinion, the whole design consists of two things: Information about my storage and some buttons to import and export this data. Thus you should consider the following when writing your markup:

    • In your .controls-box, you can't use divs where you must use buttons. Why? because they are controls, they have functionality:

       // THIS IS WRONG
 <div class="document">
   <img src="images/icon-document.svg" alt="" aria-hidden="true">
 </div>

 // DO THIS
 <button type="button">
   <img src="images/icon-document.svg" alt="Save your data">
 </button>

    • It would be great if you could present storage information using a description list (dl). I know It would be hard to match that with the design. But I would do that under any cost using the .visually-hidden class and the aria-hidden attribute.

    Marked as helpful
    2
  • SheikBazith•240
    @SheikBazithSubmitted 11 months ago

    Any suggestions on this would be greatly appreciated.

    NFT Design

    2
    Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hello there,

    I think you did a great job with this challenge. I just have some notes that might find helpful:

    • You need to increase spacing between elements a little bit as, right now, your elements are crushed into small space.

    • Your solution lacks some features when it comes to accessibility. You should wrap the whole card in an article element as it seems like a standalone element (it gives meaning on its own).

    • In a real-world application, the text "Equilibrium #3429" won't be your h1. Therefor, I suggest using an h2 or h3 for that.

    • When presenting the price and the days left, you can use a dl element whereas hiding the terms using a .visually-hidden class, so that it makes sense for disabled users. It should be something like that:

      <dl>
  <dt class='visually-hidden'>Product price</dt>
  <dd>0.041ETH</dd>
  <dt class='visually-hidden'>days left</dt>
  <dd>3 days left</dd>
</dl>

      You can find more about the dl element here. You can also read about the .visually-hidden class here

    Marked as helpful
    1
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Al Salam 3likum Abdulrahman,

    • Regarding your question about the empty space at the bottom, I think you just forgot to set the line-height property on your fifth card's quote. The problem is in this CSS rule:

       // Try adding (.card-5 .review-quote)
  .card-1 .review-quote, .card-2 .review-quote, .card-4 .review-quote {
  color: rgba(255, 255, 255, 0.75);
  line-height: 1.65;
 }
 // More better, you can do something like this:
  .card .review-quote {
  // Your props here
}

    I've noticed some other issues concerning accessibility and code structure. If you are interested in solving them, just let me know.

    Thanks.

    Marked as helpful
    1
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hi Alain,

    Regarding your question about better CSS, I wanna say there's no best way for doing that, but you should follow these guides in general:

    • I see you are using Bootstrap. But if you are at the beginning and you want to write cleaner CSS, you should try to make things on your own. Practice makes perfect. When you reach a point where you think you've got a good grasp of CSS, then you can use libraries like Bootstrap and Tailwend.

    • Try making use of CSS Custom Properties (Variables) as they make your code reusable

    • When adding class attributes, try to make them as descriptive as possible. For instance, class attribute with values like "div", "one, two" or any generic name are not preferable. You can also use the BEM Naming Convention

    • Kevin Powell has a great channel that is mainly interested in CSS stuff. Personally, this channel helped me a lot.

    Besides that, there are some accessibility issues that I can mention if you are interested.

    I hope you find that helpful.

    Marked as helpful
    1
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hi Asma,

    Again, I do believe you are really talented when it comes to applying the UI. I like how it's responsive on all screen sizes. I just have some notes here that you might find useful:

    • Text like '30-day, hassle-free money back guarantee' should not be wrapped in an h2 element as it's not actually a page heading. Use a regular p element instead as headings imply the beginning of a new section.
    • Try to make your classes names more descriptive so that other developers can test your code more easily. For instance, you can replace your one, two and three classes with intro, cta (call to action) and outro or anything that is meaningful.
    • Again, you should've used a description list dl for the pricing.
    Marked as helpful
    0
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hello Osman,

    So I think your solution is great regarding the UI and even some of the accessibility features. I've noticed you are using the article element for your card, which is great. However, there are some subtle notes that you might find helpful:

    • The alt text of your image should be descriptive. Placing the text (Perfume Image) as the alt text is quite generic. Instead, try using a longer version that clearly describes the image. Something like: 'Gabrielle Essence Eau De Parfum surrounded by tree leaves....etc'. You can read more about the alt attribute here

    • In the price section you can replace your div.felx-group with a dl which can take key-value pairs to associate them together. That would be something like this:

    <dl>
  <dt>Product Price</dt>
  <dd>$149.99</dd>
  <dt>Original Price</dt>
  <dd>$169.99</dd>
<dl>

    Thank you.

    Marked as helpful
    0
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hello Asma,

    Generally, I think you've done a great job. Your solution looks good and close to the original design. However, I've some subtle notes that you might find helpful:

    • Your solution lacks some features when it comes to accessibility. You should wrap the whole card in an article element as it seems like a standalone element (it gives meaning on its own). When presenting the price and the days left, you can use a dl element whereas hiding the terms using a .visually-hidden class, so that it makes sense for disabled users. It should be something like that:

      <dl>
  <dt class='visually-hidden'>Product price</dt>
  <dd>0.041ETH</dd>
  <dt class='visually-hidden'>days left</dt>
  <dd>3 days left</dd>
</dl>

      You can find more about the dl element here. You can also read about the .visually-hidden class here

    • This is not important, but you should add some padding to the top and bottom of the page. It would also be great if you made effects (on hover state) have some duration using the transition-duration property

    Marked as helpful
    0
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted 11 months ago

    Hi Alain,

    1.Concerning the dropdown menu issue, your main problem is that you are giving the div.dropdown a position: relative, which makes your dropdown tied to that div. Instead remove the position: relative, and try to center the dropdown using left: 50%; transform: translate(-50%); with some padding. For more info on how to implement that, Consider having a look at my solution

    2.I've noticed that your navigation links are not wrapped in a a element, which is bad regarding the functionality and accessibility of the app. So, instead of <li>Home</li>, try something like <li><a href='/home'>Home</a></li>

    3.You should edit your headings (h1, h2, h3, ...) a little bit. You can not have an h6 and then an h4 right after that like in the article cards. In fact, text like, By Claire Robinson should not be a heading. It's more like a subtitle. In your markup, you can place it after the main heading (the article title) and then reverse their order in the layout using the order property.

    I hope you find that helpful.

    Marked as helpful
    1
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted over 1 year ago

    Hi, I think you did a great job with the design, but you have some issues with naming things....

    • For instance, we don't usually use the underscore (_) to separate words when naming classes like in main_container. Instead, you should use the hyphen as main-container.
    • you can also change the name of some classes to a more meaningful ones, so classes like main-container, content and plans can be replaced with card, card-content, card-plans. That way it will look more connected to each other.
    • It might be a good idea to give your h1 a class of card-header and your p a class of card-description.

    I hope you find that helpful.

    Marked as helpful
    1
  • vivekrajput@93•450
    @vivekrajput-93Submitted over 1 year ago

    Hi there 👋, I’m Vivek Rajput and this is my solution for this challenge.

    🛠️ Built With:

     HTML5,
 CSS3,
Reactjs.

    Any suggestions on how I can improve and reduce unnecessary code are welcome!

    Thank you

    Huddle-Landing-Page.

    2
    Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted over 1 year ago

    Hi,

    • I've noticed that you haven't applied any of the hover effects in the page, so try to apply it. You can use the fill property with SVGs to do that.

    • Try to have a main element (only one) in your HTML to improve accessibility (instead of your container for instance), and don't nest it into any parent as it represents a landmark.

    • It's advisable to use semantic elements with meaningful identifiers. For instance, classes like left and right can be replaced by hero-page-img and hero-page-text

    0
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted over 1 year ago

    HI,

    • Concerning your question, you can change the color of the SVG using the fill property in CSS. So, you can make something like:
           svg:hover {fill: red;}

    If that doesn't work, try to remove the element's intrinsic style in the HTML code. You can also use icons that are easier to style from libraries like FontAwesome.

    Marked as helpful
    0
  • Ahmed El_Bald•1,020
    @Ahmed-ElbaldPosted over 2 years ago

    Hi Tertsegha,

    I think the design is great. But u have some issues with accessibility:

    • Try to have a ‹main› in the page (only one) and don't nest it inside any elements.

    • Don't add styles on elements in the HTML code. It's not a good practice and it's causing some issues.

    Thank You

    0