Skip to content
OverviewSolutions
22
Comments
15
Shuaib
@JustShuaib

All comments

  • Shuaib•640
    @JustShuaib

    Hi there. On desktop, the shop now button falls into the image below it,maybe you should find a way to fix it. I also noticed that your slider works but once it reaches the ending,it stops working. It would be nice if it could go back to the first image. Also, one of the requirements of the challenge is that the slider should be navigatable using the either the mouse or the keyboard. I can't navigate this using the keyboard. Your banner looks stretched both on mobile and desktop. Maybe reduce the height on mobile and reduce the width on desktop.

    Nice work tho, and happy coding.

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there. I noticed that after searching for a country, there is no way to get the full list of countries again. I feel it will be a better user experience if the countries filter as user types instead of pressing enter before they are filtered.

    Also, on the individual country page, the back button should go back to the home page; instead of the previous country's page.

  • Shuaib•640
    @JustShuaib

    Hi there, I noticed that while searching for countries, if there is let's say only two countries that match the search criteria, there is usually a space between them. You can search for korea to better see what I am talking about.

    Also, if I am searching and there is no country that matches my search parameter, it will be nice to have a text that says country not found or something similar. Instead of a blank screen.

    It also seems like the filter by region isn't showing the full country list. It's only showing 8 countries when I filtered Africa. In fact, the landing page shows only 8 countries.

    Nice work though

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there. I must say I love your solution, It's really good work. I would like to point out some things though:

    • Firstly, maybe you did not take note but your font is different from the one in the disign. Also, I noticed that the search and the filter are not synced. Like when I'm in Africa and I search for China I still brings it. China is not part of Africa.

    Within each country's detail, clicking on the border country is supposed to take me to the page about the country. It doesn't.

    Also, after filtering based on region, there is no way for me to go back to the full list of the countries.

  • Shuaib•640
    @JustShuaib

    Hi there, concerning your question, you used input type of url. You could change it from url to text and then perform the validation yourself (using regex here)

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there, I noticed I can't switch from theme three to theme two in your application. You should try to work on it. Also, if I divide a number by zero, the result is infinity. If I want to perform another operation after that, it adds infinity to whatever I type in. Same thing if I divide zero by zero. Also,If I have a result greater than 15 characters, it enlarges the screen. Was that intentional or a mistake?

    Going through your CSS too,you should make it one selector per line. For example, you have this.

    body[data-theme=theme-1] .calculator main #button-delete, body[data-theme=theme-1] .calculator main #button-reset, body[data-theme=theme-1] .calculator main #button-calculate {

    Add line break after each comma.

    You had quite complicated selectors too, for example

    body[data-theme=theme-2] .calculator header .theme-switch-container .theme-switch-labels .theme-switch-label {

    Chances are having complicated selectors like this is bad practice, try to keep it flat. This is a very nice solution regardless.

    Happy coding! 😊✌️

  • Shuaib•640
    @JustShuaib

    Hi there, you should work more on the active states, e.g when the input fields are in focus, they should have border and when the input is zero, they should display an error stating can't be zero

    The way the custom button works is rather confusing. You should include a focus state on it too.

    Also, the reset button does not seem to be working.

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there, I noticed from your solution that the input-people displays can't be zero when either of the input field is filled. You can work on it to make it display can't be zero only when the input entered into it is zero.

    Also, the two inputs have a green border on hover. Instead of hover, you could use focus. Really nice work I must say 😊

  • Shuaib•640
    @JustShuaib

    Hi Zin Hein, I must say I'm literally blown away by your solution. I so much love it. I wanted to point out though, that the reset button does not reset the custom input. Probably you didn't notice.

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there! I just wanted to point out a few things. While fetching the data from the json file, always include a way to catch errors incase the request fails to be successful.

    You had

    document.addEventListener("DOMContentLoaded", () => {
// function async to get data from the json file
async function loadObjJson() {
const response = await fetch("./assets/js/data.json");
const data = await response.json();
console.log(data);

    Declare a function in a separate block then call the function inside the DOMContentLoaded event listener,instead of including the function block inside the event listener. Also, always remove console logs from your code when you're done

    Also for your titles, instead of selecting them individually,you could give all the headings the same class and querySelectorAll all the headings then loop through the data to display each in the respective title.

    Happy coding!✌️😊

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi there Mohammed, going through your solution now and wanted to point out a few things. Looking at the design on a desktop, the last week and 32hrs seems to overflow to the next line. Try reducing the font-size of the 32hrs. Upon clicking the daily, weekly and monthly buttons, the number of hours didn't change. Either it's intentional; check through the README, the number of hours are supposed to change when the buttons are clicked. Aside these, this is very good.

    Going through your codes now and I noticed you didn't include JavaScript. I'm guessing it's intentional. Also, all your contents are inside divs and there is no landmark tag surrounding your content. You should consider using a main tag. You can read more about landmark tags here

    Your CSS is really neat. Nice work ✌️

  • Shuaib•640
    @JustShuaib

    Hi there, going through your solution now and I wanted to point out a few things.

    Always wrap all your contents in a (landmark tag)[https://www.w3.org/TR/wai-aria-practices/examples/landmarks/HTML5.html] .

    You used an h1 and h4. You do not have h2, h3, h4. Ideally, your headings should follow a descending chronologic order. i.e after h1, you should have h2 and not h3 or h4. Instead of using a h4 here, you could use a p or span.

    Also, you change btn's font is arial. You should include font: inherit to the button or use a different tag. An a should be ideal here.

    You included

      
  html {
    font-size: 40%;
  }

  .card {
    flex-shrink: 0;
  }
}

    at the end of your code. Generally, it is better to include your media queries close to the class/element you're adding the query to as it is easier to debug.

    You should also look through the accessibility and HTML issues after submitting your response on FEM, it gives you a heads up on possible things to imporve.

    Happy coding!

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Thank you so much Ivan, I really appreciate you taking your time and going through my code. 😊 For the animation, I actually saw it on MDN used that way, that was why I used it. But I've changed it.

    By moving the function declaration outside the event listener, you mean I should define the function elsewhere and call it inside the event listener right? I'll handle the errors now too. I didn't think of it while implementing the code😄 I really appreciate your feedback and I'm definitely upvoting!

  • Shuaib•640
    @JustShuaib

    Hi Robin. The card seems quite big on a desktop. Consider reducing the size a bit? The animation & bounce effect also feels nice! 😊

    Then you asked some questions:

    • When I worked on mine, the part where I had issue was the effect on the list. Removing the active color from a button when another button is selected.
    • I actually used vanilla JS for the project 😀. I think I used SASS for the styling.

    Happy coding!

    Marked as helpful
  • Shuaib•640
    @JustShuaib

    Hi Aayush. I really love your solution and how detailed it is. It is a job well done I must say! For the underline on the nav bar items, you could add an after pseudo element to each link item and then use position absolute to properly place it (or at least that's what I did 😁). Then you should probably add transition effects on the links to give it a nice transition feel. Also, for the hover effects,I think the lime green is closer to the design than the cyan. I was confused about this at first too😂.

    Marked as helpful