Skip to content
OverviewSolutions
18
Comments
25
P
Jeremy Helsel
@JIH7

All comments

  • Eric Teye•40
    @ericaimhigh1Submitted about 1 year ago

    I had fun working on this project. It was pretty simple as I have worked on more complex stuff through tutorials. For this project, there's nothing I can say I am unsure of.

    QR Component using CSS

    2
    Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Looks great! Like you said yourself, it's a pretty simple project, not a ton to improve on. The only thing I noticed in your code is there's an unused media query in your CSS.

    1
  • Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Looking sleek and mobile friendly! Good semantic HTML markup and style rules. Really any feedback I could give are super small nitpicks. Starting with mobile styles and using media queries for tablet/desktop is generally considered a better workflow than starting with desktop and working your way down. It's something I've only somewhat recently started doing myself. The only other thing that I think could really help this project shine is a custom readme! Right now you've got the default Frontend Mentor README.md showing on your GitHub repo, and adding your own does wonders for making the project look better. I always find the README-template.md that comes with these projects to be a great jumping off point.

    Marked as helpful
    0
  • Steven Nguyen•30
    @tunabearfishSubmitted about 1 year ago

    I had trouble getting my summary section, had to play around more, I think there are more efficient ways of doing this, but this was what I was able to do

    HTML and CSS, FLEXBOX

    3
    Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Looking good overall! One issue I noticed, I believe you're trying to select the body element with the selector .body. The dot at the begginning makes this a class selector. So it would work for <body class="body"></body>. Changing it to just body would apply the styles in the way I think you're intending to. I downloaded your code and tried that and it put the container nice and centered on the screen.

    I've also noticed you have a white box shadow on the container, causing it to blend in with the white background. When a color is added, the borders of .container extend past the content. To get the box shadow working properly, try giving .container a proper width instead of a full screen width of 1440px. Something like 736px is more akin to the design. You will of course have to change the widths of the objects in between to avoid them squishing down since you're using percentages, but the page will look better because you did it. You probably also want to change the justify-content of .container to space-between. The flex rules you've set up on body will keep the container centered on the screen for you.

    You've got a solid start here and with some tweaks it could look really great! After that, try challenging yourself to learn CSS media queries to make it look good on mobile! That way, you can selectively apply CSS styles based on the screen size. Here's an article about that. Remember to hit f12 and open your browsers dev tools, then select different screens under "dimensions" at the top to see how it would look on different phones and tablets. Best of luck! Feel free to respond with any clarification questions

    0
  • Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Hey! Awesome that you're starting your journey as a front end developer. Right now, there are a lot of ways you can improve this project. Your style tag isn't within the head tag, which could cause some issues. It also appears you're using qrcode.html file mostly for style, then loading the index.html file into an iframe. This is a pretty unusual approach and wouldn't really be considered best practice by most. If you would like to seperate your styles from the content, consider making a .css file and importing that into your html file with a <link> tag within the <head>. You can put your styles in there.

    The content in index.html also appears to be untagged. Just like with your body, iframe and image tags, you generally want all of your text to be within tags too. For instance "Improve your front-end skills by building projects" would be great inside of a heading 1 or <h1> tag. So something like <h1>Improve your front-end skills by building projects</h1>. The second line of text is great for a paragraph or <p> tag. This makes it easier for search engines like Google to understand your website, helps people with accessibility needs navigate your website, makes things easier to style, and will make it easier for you to work on a project you haven't touched in a while.

    I noticed you have a hover effect on the image, that isn't really required for this particular project if you would like to make it look like the design image.

    Also you can round the edge of the image with the css border-radius property. Something like border-radius: 10px; on the image would make it look more like the one in the design.

    I made a version of this as well if you'd like to see the code. Ignore the .scss files, that's a tool I used to help me write css and not something to worry about yet. The important files here are index.html and style.css. Live version of the site here.

    Let me know if you have any questions I can help with. Best of luck to you! Keep learning and you'll be making beautiful web pages before you know it :D

    3
  • Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Looks great! One small oversight I noticed is that the price text is supposed to be green instead of gray. Otherwise very solid job replicating the design! Looks good on mobile and hover state on the button works well too. Great job!

    Marked as helpful
    0
  • Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Overall looks good! I noticed you're not using the same font as the design. Check the included style-guide.md file for fonts used in the project. Then you can get whatever font you need from Google fonts and import it in the head of your HTML. Google fonts will actually prepare the necessary link statement and CSS rules on the right side of the screen.

    Also the heading on the white tiles looks like it's supposed to be some shade of gray instead of black and the heading text should be a little smaller. Colors used should also be in the style-guide.md. Great work!

    Marked as helpful
    0
  • BrovoJD•190
    @Jaheim-DouglasSubmitted about 1 year ago

    This challenge was easier than I expected it to be, but it's good to refresh on using flexbox, inline-block and centring divs.

    centring div's

    1
    Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    The link to the Github repo doesn't appear to work so I can't see your code, but if you're trying to center the "container" div on the screen, you'll want the body to have a height equal to the screen height. HTML elements will automatically grow wide enough to fill the available space, but will only grow as tall as they have to to fit all of the content inside. Setting the body tag to the height of the viewport helps with this.

    body {
    height: 100vh;
    display: flex;
    flex-direction: column;
    justify-content: center;
}

    A vh is equal to 1/100 of the screen height and a vw is 1/100 of the screen width. You don't want to overuse these units as it can cause problems, but putting them on the body is generally fine.

    0
  • Jeremy Helsel•670
    @JIH7Posted about 1 year ago

    Looks good on desktop! I did notice the layout isn't currently responsive so it doesn't look right on mobile. If you're not familiar with media queries, I suggest checking them out.

    Here's an article on them

    Making sure your site looks good on mobile is really important since that's the way most people engage with the web.

    Also if you add a hover effect on the button it will go a long way towards making things feel nice. Here's an easy way to do that in your CSS.

    .buy:hover{
    background-color: hsl(158, 42%, 18%);
}
    1
  • Alina•150
    @electr0spaceSubmitted over 1 year ago

    Hello amazing community! 👋

    I'd like to ask you a question: what are your solution for displaying one image for desktop and another for mobile?

    Decided to simply go with :

    .mobile-img {
    display: none;
}

    @media only screen and (max-width: 745px) {
    .desktop-img {
        display: none;
    }
}

    What do you think? 😅

    Responsive product card with HTML/CSS/JS (interaction with button)

    3
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    This looks great! I really love the touch of the button getting larger on hover and the dialogue window for clicking. Very satisfying.

    I think the way you're swapping the images is perfect, and it's also what I did for this challenge. It's mobile first which is great, and it's probably the simplest way to do that. Sure there'll always be an extra <img> tag in the hierarchy, but nevertheless this is the best way I've found to do this both in terms of performance and complexity.

    Marked as helpful
    1
  • MB•20
    @mariabpulgarSubmitted over 1 year ago

    The most difficult part for this project was the responsive part. My code works well in the browser and when I resize the screen in the inspector, but when I open it in my phone (iPhone 14 Pro) it doesn't work. I would really appreciate any advise.

    Responsive design of a product card component

    1
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    It actually looks good for me on Firefox for Android! Wish I had an iPhone to test it on and see the issue myself.

    Some other things worth noting though, practicing semantic HTML is always a good thing to do. This is just a small component so there's not a ton of use for semantic tags, but one major one is that the <div> with the .main-container class should maybe be an <article> instead. Using semantic tags instead of regular <div>'s when applicable is great for people using screen readers.

    Here's an article on semantic tags!

    Also it's generally good practice to avoid !important in CSS. Try using a more specific selector instead when possible.

    Marked as helpful
    0
  • BBualdo•540
    @BBualdoSubmitted over 1 year ago

    FIXED AND DONE

    Hi everyone!

    I spent so many hours on this and I can't progress any further. Probably it's just a lack of knowledge or I don't understand some JS behaviours 😢

    I designed it pretty fast, I have no problems with HTML and CSS, it's like breathing. Even if I don't know something I can google it easily. I made up my own idea of segregating and styling dark/light theme. Check it out and let me know if it is a proper way.

    Then I programmed appearing text from Input on the list. Easy.

    But when I tried to code delete buttons, my nightmare has started. I red, watched and tried 'normal' methods, array methods... Finally after 3hours I just coppied code from my previous 'Todo', which I was doing with one of the courses I watched. I tried to understand what I've just coppied, but Objects and Arrays are some black magic to me. I understand how to write them and which one is which, but I just can't use them by myself to code anything. I imagine that coding Completed, Active and All sections requires using 3 arrays, if I'm right... but how?

    I wish I could learn this OOP in JS. But I can't find tutorials or courses that could explain to me in a way I can understand and remember...

    This is my first solution in Intermediate level, but I can see now that it was too early for me 😢

    If you have any sources of knowledge or some lessons I will appreciate your effort!

    Thank you! ❤

    EDIT #1: I fixed markAsCompleted function and now it adds class to each checked todo. I also implemented Clear Completed button logic, where it checks if todo-list has class named 'completed' and if it has, it should splice that todo from an array and render list without that todos. But... Check it out what is happening. I have no clue. I'm not even sure if renderTodoList() is appropiate function.

    Most difficult project so far!

    3
    Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Hey, this looks really awesome. It can be very frustrating when you can't get things working quite right, but the HTML and CSS are on point and you have a lot of the functionality there.

    I think a great resource for learning arrays and OOP (and where I learned them) is the 100devs course by Leon Noel on YouTube. You can probably skip the early classes because your HTML and CSS skill are absolutely on point.

    Practice Arrays

    Object Oriented Programming

    You mentioned not understanding how your delete button works, let's take a look at it. Your current solution is concise, well formatted, and would probably be the way to do it in production for a company, but lets do a refactor along the way to make it easier to understand.

    document.querySelectorAll('.delete-button')
    .forEach((deleteButton, index) => {
      deleteButton.addEventListener('click', () => {
        todoList.splice(index, 1);
        renderTodoList();
    });
  });

    You start by calling document.querySelectorAll('.delete-button'). This goes through the document and finds ever item with the .delete-button class and adds them to an array. For readability, lets store that array in a variable instead

    const deleteButtons = document.querySelectorAll('.delete-button');

deleteButtons
    .forEach((deleteButton, index) => {
      deleteButton.addEventListener('click', () => {
        todoList.splice(index, 1);
        renderTodoList();
    });
  });

    Then you call array.forEach() method on the array. .forEach is a "higher order function," meaning it takes another function as an argument, which you're currently doing with an arrow function. Let's abstract that out into it's own function to make it more readable for us.

    const deleteButtons = document.querySelectorAll('.delete-button');

  deleteButtons.forEach((deleteButton, index) => addDeleteListeners(deleteButton, index));

  function addDeleteListeners(deleteButton, index) {
    deleteButton.addEventListener('click', () => {
      todoList.splice(index, 1);
      renderTodoList();  
    })
  }

    You do still have to use an arrow function to get the arguments but I think this makes things a little more readable for this. .forEach will loop through our array and perform a function on each item within it. In the arrow function of a .forEach the first argument is always the item itself. You can name it whatever you want, in this case it's deleteButton but you could name it element, el, e, zebra, fdsfjdsahfksldah etc. You can actually just have one argument if all you need is the item. So that would look like deleteButtons.forEach((deleteButton) => addDeleteListeners(deleteButton,)); We will, however, need the index as well. Since arrays are a numbered list of objects, index represents an item's position in the array. Indexes start at 0 so the first item's index will always be 0, not 1. We can actually access an individual item with the index using square brackets notation.

    const deleteButtons = document.querySelectorAll('.delete-button');
console.log(deleteButtons[0]);
console.log(deleteButtons[1]);
console.log(deleteButtons[2]);

    This will output each the first three buttons' HTML markup to the console. So index will be the second argument of .forEach. We can technically name it whatever we want, but generally just index or i is a good practice. Fun fact, you could add a third argument to this function, and that would be the array itself. Once again you can call it whatever you want. So deleteButtons.forEach((deleteButton, index, array) => addDeleteListeners(deleteButton, index, array)); We don't need to do that for this particular case however.

    So onto the main part of this function.

    deleteButton.addEventListener('click', () => {
      todoList.splice(index, 1);
      renderTodoList();  
    })

    I won't explain event listeners as you already seem pretty comfortable with them, but note that since we are iterating over each element, it is adding an event listener to every object in the array.

    Within the event listener you're using array.splice() on your todoList array. .splice is a function that removes an item from the array at a specific index. This is where our reference to the index comes in handy! Since every item in the todoList array has a delete button, todoList and deleteButtons will always be the same length, and because .querySelectorAll reads from top to bottom, they will always be in the same order! So .splice removes things from the array at a specific index, for that we'll just user our handy dandy index value. The second argument is how many items to remove. Of course for our purposes we only need to remove the item that is being deleted, so we just have that be 1. I hope this explanation helps!

    I bet you could totally get the clear all button working! You very similarly use a .forEach with an if statement inside the function. in a statement like todoList.forEach((todo, index) => {//Your code here}), you can check if todo has the .completed class and splice it out if it does. Best of luck and keep up the awesome work!

    Marked as helpful
    1
  • Jeremy Helsel•670
    @JIH7Posted over 1 year ago

    Looks good! I would give the text on the buttons and the star image user-select: none so that they can't be highlighted.

    Looking at your script, I think it would make your code more readable if you used camel casing. Instead of howcontainernumber saying howContainerNumber makes things more readable. General convention is you have the first word lowercase, capitalize every word after. Camel casing is the most common way to go in the JavaScript world but you can use other kinds like snake casing how_container_number or Pascal casing HowContainerNumber (same as camel but the first word is capitalized too.)

    0