Skip to content
OverviewSolutions
11
Comments
20
P
Grog the Frog
@GregLyons

All comments

  • Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Great work! You've completed quite a few complex projects recently. This one runs smoothly, and you handle state and asynchronous data fetching nicely.

    On your next project with asynchronous data fetching, I recommend reading this resource about handling loading and errors. That author also writes a custom hook to handle such things, which is good to read.

    A very good library for handling such things is React Query. In the example in that link you can see the useQuery hook similar to the first link. This library also handles a lot of other things for you, like caching (so you'd only need to do the "All countries" query once, and then when the user returns to that page it'll use the cached data instead of running the query again) and deduping (if you have multiple components on the page that call the same query at the same time, the query will only be done once, with its results being shared to both).

    One thing I would suggest for your code is to use more semantic HTML. In a lot of caes you're using <div> to contain text, whereas something like a heading <h1> (the "Where in the world?") or a <p> (for a lot of your other text) would be better. I think the best place to start would be the Text Content section of this MDN resource. I think that section's the most important (as well as "Forms"), and by practicing those two sections you would address a lot of the accessibility/HTML issues that Frontend Mentor is flagging.

    Marked as helpful
    2
  • Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Congrats on your first challenge! Your solution looks good. Here are some tips for future challenges:

    1. Consider using a CSS Reset. Many elements in HTML have some default styles applied to them, like some top/bottom margin on <p> tags. In this case, those margins end up being in line with the design, but oftentimes you may find yourself struggling to remove a lot of default stylings to get your work to look more like the design. A CSS Reset like the one above should alleviate this, and give other more sensible defaults to start from.
    2. Your card wrapper has a width: 20%. This looks good on larger screens, but as you shrink the screen horizontally the card gets too narrow too quickly. Something like minmax or clamp are helpful to give a minimum and maximum width for your content, which will make your designs more responsive.

    Good work again, and best of luck on future challenges!

    Marked as helpful
    0
  • Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Good work! You're using semantic HTML (<h1> for heading, <p> for body text), which is all you need in terms of accessibility for this challenge.

    I'll give you a few pointers I keep in mind when thinking about accessibility. Since this is your first challenge, I can't really assess what you know in terms of accessibility, so forgive me if I'm saying things you already know. My tips will mainly revolve around using semantic HTML, which is the idea of writing your HTML in a way that can be more easily understood by machines (e.g. screen readers for human comprehension, search engine crawlers for SEO, etc.).

    1) Headings Semantically, <h1>-<h6> elements denote sections on your web page. You should have precisely one <h1> on any given web page, which serves as its title. Then, your heading levels should decrease by one. That is, the heading of every section thereafter should have an <h2>, the subsections of those sections should have <h3> headings, and so on. A common mistake is that people will look at the appearance of a heading in a design, and assign it to <h3> or <h4> based on that, when semantically (i.e. the meaning), the heading introduces a main section of the page; in this case, it should actually be an <h2>. Remember that you can always style headings as you desire, so the HTML (i.e. which heading element) you use should reflect the meaning of the heading on the page, not the visual design you've been given.

    Another reason this is useful is that a lot of screen readers allow users to navigate the page by going to different landmark elements. For example, they can go to the next <h2>, or the next <h3>, etc. Thus, if two sections are the same level in the logic of the document, you should give them the same heading level (even if the headings are styled differently).

    2) ARIA roles To help screen readers understand your web page, there are many different attributes like aria-*, e.g. aria-role. For example, if you put an aria-role="button" on a <div>, screen readers will interpret the <div> as a button, which then indicates to the user, who may have trouble/may not be able to see your web page, that they can press it. Without this role, there's no indication (aside maybe visual indication) that the <div> can be interacted with.

    The thing is, different HTML elements have certain aria-* properties by default. For example, a <button> element already has aria-role="button" built in, so screen readers will interpret as a button without you needing to remember to add the appropriate ARIA role! Thus, using semantic HTML makes it a lot easier to make your websites accessible.

    3) Lists Think about which content on your page is semantically a list. For example, in a lot of Frontend Mentor challenges, you'll see a series of cards on the landing page describing the features of a product. I like to make these into lists using <ul> for the parent container and <li> for the individual cards, instead of using <div>s in either case. Why? Well, the <ul> now gets aria-role="list" automatically, and each <li> gets aria-role="listitem", so screen readers can now interpret these cards as a list, which is what they are semantically: a list of features for your product.

    Another neat things is that, upon entering the list, screen readers will signal to the user that they're entering a list, and it'll even tell them the number of items. Imagine instead that you have a list of <p> tags for the features of a product. A screen reader would read them one after the other, without telling the user its a list. This leads to confusion (imagine you're being read the words aloud without any breaks between items), and using an unordered/ordered list prevents this.

    I hope this helps! If so, feel free to mark this comment as helpful. Either way, I hope these pointers will help you write more accessible web pages. Best of luck in your future projects!

    Marked as helpful
    1
  • Grog the Frog•480
    @GregLyonsSubmitted over 2 years ago

    It was a bit tricky styling the inputs at first, as I wanted to use <input type="radio"> rather than <button>s. Let me know if there's a simple way to accomplish the design (while still using `<input type="radio">, which I feel is the most semantic element for the task). Any other feedback also welcome.

    Learning Tailwind 3

    1
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    (Doesn't look like I can edit my solution question, so I'll post a comment instead.)

    For some reason it says "SUBMIT QUERY" on the button in the Frontend Mentor screenshot for my solution, but when I go to the site link it just says "SUBMIT". Also, I don't have the word "query" anywhere in my source code. If anyone knows what this could be, let me know!

    0
  • Keagan Dickinson•160
    @PhisherFTWSubmitted over 2 years ago

    I found, the transition when hovering over the image difficult, it took me several attempts and a lot of looking up on StackOverflow, although I finally got a working solution. I am still not a 100% sure exactly how it works, and I wouldn't be able to reproduce it from memory. Please if you have any recommendations to simplify and make my code better let me know.

    Basic NTF Preview-card

    2
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Good work!

    I actually think your solution to the tricky image-hovering problem is pretty good. When you say you're not entirely sure how it works, is it the CSS that you wrote that you're having trouble understanding? The hover effect is achieved using the :hover selector. The key is your two rules:

    .container:hover .image-equil {
opacity: 0.3;
}

.container:hover .middle {
opacity: 1;
}

    The rules are stating the following:

    1. When .container is being hovered, reduce the opacity of any of its .image-equil descendants.
    2. When .container is being hovered, increase the opacity of .middle (which is otherwise at 0) to 1.

    I believe what's happening here is this: the background of .container is that teal color, so when you reduce the opacity of .image-equil--which is taking up all of .container, the background color shows through (Rule 1). Then, the content of .middle consists of the eye image (which you've centered using CSS), and when hovering the eye becomes fully opaque, and actually blocks the portion of the .image-equil that is under the eye image (Rule 2). This is the desired effect, as you don't want to see the NFT image through the white eye (according to the design).

    I don't think it's necessary to have the .middle <div>, as you already have the .middle class on the eye <img> itself, so even without the <div> it'll still be a descendant of .container, and Rule 2 will apply to it. Other than that, I don't see much way to simplify your image hovering thing.

    Two more things I want to point out:

    1) For the author name "Jules Wyvern", you have a separate <p>, which you set to display: inline; so that it'll be on the same line as "Creation of". There's already a specific HTML element for what you're trying to do there, however: the <span>. Using a <span>, you'd have

    <p class="creator">Creation of <span class="name">Jules Wyvern</span></p>

    Since <span>s are inline elements, there's no need to change the display property in that case. This is also better since, if you think about the problem in terms of what the HTML elements you have are saying, they're indicating that "Creation of" and "Jules Wyvern" are in separate paragraphs, which is not what you're trying to convey. Instead, you want "Creation of Jules Wyvern" to be a single paragraph <p>, and then you can change text within that paragraph using a <span>.

    2) When you feel ready, I recommend learning Flexbox as soon as you can. It'll make solving these UI/layout challenges a lot easier. For example, your CSS:

    .eth-txt {
    position: relative;
    display: inline;
    width: 2rem;
    padding-right: 3.25rem;
    color: hsl(178, 100%, 50%);
}
.days-left {
    display: inline;
    position: relative;
    width: 2rem;
    color: hsl(215, 51%, 70%);
}

    which you use to position the ETH text and days left, only works for the very specific width of the box. If the width of the .card-body element were to change at all, suddenly your padding-right on the .eth-text would no longer make it look like the two pieces of text are on opposite sides of the same line.

    A more robust (i.e. works at any screen size/container size) would be to put your .eth-text and .days-left in a wrapper <div class="wrapper"> with

    /* Selects the .wrapper <div> */
.wrapper {
  /* Turns .wrapper into a Flex container */
  display: flex;

  /* Direct children of .wrapper placed on opposite sides/spaced out evenly up to the edges */
  justify-content: space-between;
}

    (Note that you'd need to put the ETH icon in the same block as .eth-text to make this work--but this would be the solution without the icon.)

    I hope this helps. If so, feel free to mark this comment as helpful. Either way, best of luck in your future projects!

    2
  • Bence Zámbó•510
    @zambobenceSubmitted over 2 years ago

    I found it quite challenging to position the profile picture in the middle, at the end I have decided to position it absolutely. I am very interested if there are any more dynamic ways to position it.

    I could not figure out how to set the background of the body so I just used a gradient, although aI would really like to know the solution for that.

    Card Component Main using flexbox

    2
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Good work on using semantic HTML. Here's what I think you can do for the profile picture.

    First, I'll point out then when I try to adjust the height on my computer, the alignment gets messed up, and the profile pic is no longer properly centered. I believe this is because of a height/margin mismatch between your .card-top, height: 20vh; and your .profile_pic, top: 23%; properties. The issue is that the 23% refers to 23% of the .card height, whereas the 20vh refers to the screen itself. The former depends on the content of the card, whereas the latter depends solely on the viewport height. I'd recommend changing the .card-top height to some sort of percentage instead, so that it depends on the height of your actual content itself. This'll prevent this weird behavior where things get out of alignment.

    As for positioning the profile pic, I'd still use absolute positioning, but in a different way:

    1) Re-work HTML Put your .profile-pic <img> inside your .card-top <div>, and put position: relative; on .card-top instead of on .card. This is so that you can position .profile-pic relative to the .card-top instead of .card.

    2) Center horizontally Check out thgaskell's answer, as it has some good info on left/right vs. margin-left/margin-right. You may need to explicitly set the width of your element in this case. If you don't want to specify the width (it's not always desirable to do so), check out Adel's answer instead.

    3) Position vertically This is where setting position: absolute; on .card-top instead of .card pays off. You want .profile-pic to align with the bottom of .card-top, which you can easily do with bottom: 0; (on .profile-pic). Then you need to push .profile-pic down a bit more so that its centerline aligns with the bottom edge of .card-top. For that, you can add transform: translateY(50%); to do the trick. Here, note that the "50%" in the translateY refers to 50% the height of .profile-pic, NOT that of .card-top (this is different behavior than, e.g. height: 50%).

    This solves your responsivity issue (where the profile pic gets misaligned on different screen heights), because you're now binding the positioning of .profile-pic to the bottom edge of .card-top, instead of binding it to the top of .card and then doing some percentage math to get the result on a particular screen size (a calculation which is very tricky and probably not stable across different screen sizes).

    Hope this helps, and best of luck in your future projects!

    Marked as helpful
    0
  • Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    It looks like all of your HTML, CSS, and JavaScript is pretty solid. Nice!

    The one thing I'd suggest changing would be to put the rating numbers in <button> elements. Right now, the user can only fill out your form with a mouse, since they can't focus the ratings to select them. While you could make the <li> elements themselves focusable, you'd have to worry about other things like making sure the Enter key also selects the rating using another event listener, and so on. Instead, the <button> element is focusable by default, and pressing Enter activates the click event, so it's a lot easier to work with. Doing this would make your website more accessible, not just for users who can't use a mouse, but also for users who prefer to use a mouse.

    As I mentioned before your JavaScript is good, but since you said that you're learning to write JS, let me give you a tip that you may want to consider for future projects:

    If, instead of passing an anonymous callback function into addEventListener, you first define a function beforehand and then pass in that function to addEventListener, it then makes it easier for you to remove the event listener in the future when necessary, like so:

    const handler = function(e) {
  ...
};

document.addEventListener("...", handler);

...

document.removeEventListener("...", handler);

    In other words, you can just pass in the name of the function to removeEventListener. See Adam Heath's answer on this StackOverflow post for more details.

    Obviously you don't need to do that in this challenge, but you'll likely find that you need to remove an event listener at some point in the future.

    Marked as helpful
    0
  • Francisco Catalan•10
    @catasistemasSubmitted over 2 years ago

    Hi everyone! This is my first frontendmentor challenge.

    i would hope get any feedback. Thanks

    Html and Css

    1
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Nice work! It looks you have a pretty good grasp of semantic HTML, as well as Flexbox. I have a couple notes on your CSS:

    Are you deliberately using box-sizing: content-box; (at the top of your CSS)? I believe that's the default behavior, so you shouldn't need to declare it. On the other hand, a lot of developers (myself included) prefer box-sizing: border-box;. For a lot of people, it makes reasoning about widths and heights easier, as margins and padding are factored into any specified width/height, as opposed to applied in addition to (and hence changing) the specified width/height. If you don't know about that, you may want to try it out. If you prefer content-box instead, though, then by all means keep using it.

    The other thing I'll mention is that your height: 100vh; rule on the <body> suffices to center your content vertically, but just in case for your future apps/web pages, min-height: 100vh; is more robust. In the case of your solution it wouldn't change anything, but it would allow the height of the <body> to increase to contain additional content.

    In general, you don't want to explicitly set the height of an element (unless, for example, you're trying to get some exact positioning), as different users' browsers have different font-size preferences. So even if your content fits into your element with an explicit height, these different font-sizes might cause the text to overflow your element on the user-end. Using min-height allows you to set your desired height, while still allowing for overflow (e.g. the background of the <div> would grow to contain the overflowing content in my above example). You may already know this and are just using height: 100vh; since in this challenge it won't cause any ill-effects, but I wanted to point this out just in case.

    Hope this helps. If so, feel free to Mark this comment as Helpful. Either way, good luck on your future projects!

    0
  • Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Good work! Here are some tips on aligning things horizontally:

    margin: auto; can work to center content horizontally, but it also applies to the top and bottom margins as well, as it is a shorthand property for (in order): margin-top, margin-right, margin-bottom, and margin-left. This can be a problem if you don't want to set margin-top/margin-bottom to those values. Thus, using either

    margin-left: auto;
margin-right: auto;

    or

    margin-inline: auto;

    is preferable.

    Using margins often requires you to explicitly set the width of an element, which you may not want (especially for responsive design). Flexbox gives you a more robust way to horizontally center content. On the parent element of the children you want to center, apply

    display: flex;
justify-content: center;

    To center your card (.whitebox), for example, you could put the above code on the <body>. Usually you'll probably need to space out the children (e.g. with the gap property), as they'll be put next to each other horizontally. Another solution that avoids this, again using Flexbox, is

    display: flex;
flex-direction: column;
align-items: center;

    Of course, if you want your Flex children to be in a row, you can't use flex-direction: column;. You can learn more about Flexbox here. I recommend you learn it, as it's very useful. You don't need to understand everything right away; you'll master it through using it in your future projects.

    The other tip I'd give is to work on using semantic HTML. That includes using header elements (<h1>-<h6>) for your headers, <p> elements for your text, and so on. This makes your web page more understandable to search engines (for SEO purposes) and screen readers (for accessibility). In your solution, I'd say your .text is an <h1> or an <h2> (every page should have an <h1> element, so probably the former; but if this component were placed in a larger context it'd probably be a lower-level heading), and your .text2 is a <p>.

    Using semantic HTML is also very important for forms and buttons, not just for the above reasons, but also because the <button>, <input>, etc. elements have a lot of keyboard functionality built-in by default. If you were to just use <div>s instead, you would have to code all that keyboard functionality yourself (or give it up altogether, which wouldn't be acceptable for most sites).

    I hope that helps. If so, feel free to Mark this comment as Helpful. Either way, good job again, and best of luck on your future projects!

    Marked as helpful
    0
  • Jose manuel osorio•60
    @jmblack15Submitted over 2 years ago

    Hello everyone!

    please let me know how I did, and any feedback is welcome, I would also like you to share any resources to learn how to manipulate the DOM

    Thank you very much!

    Interactive rating component

    2
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Based on your solution--which works well, by the way--it looks like you have a pretty good grasp of basic DOM manipulations, so good job there. I don't have a single resource that I used to learn DOM manipulations (and part of the reasons I'm doing these challenges is to learn how to manipulate the DOM with vanilla JS instead of using something like React). It looks like this is a good source, but the approach I'm taking right now is to get in practice through these challenges and Google whatever I need as I go. I find that I know enough about what can be done with the DOM so that I know what to Google when I need help.

    I'd say you have a pretty good foundation with all the functions you used here (selecting DOM elements by classes, IDs, etc., adding event listeners), so that in the future if you want to do something you (e.g. listen for a particular event) but don't know the syntax, you could Google, e.g. "javascript listen for <type of event> event" or something like that.

    Another couple notes I have are on your HTML/CSS:

    1) Whenever you use a <section> element, you need an <h1>-<h6> element somewhere within (probably not an <h1> since you really should only have one of those per page); the first such heading element within the <section> will be the name of that section for screen readers and other technologies that are trying to understand your page. For your case, I'd say that "Thank you!" is an appropriate header (<h2> or even <h1> since there's no other <h1> on the page).

    2) Aside from the last point, I'd say you're doing a pretty good job using semantic HTML. I do feel like you're using a bit too many <div>'s though, specifically in your card-state section. It looks like you're using, e.g. .card-container-selected to apply background-color, border-radius, and centering, but that's not really necessary. Since your <p>, .card-state-selected, is already a block element, you can apply the background color and border radius directly to that using CSS.

    Also, in this rule:

    .card-ranking,
.card-state {
  width          : 100%;
  height         : 100%;
  display        : flex;
  flex-direction : column;
  justify-content: space-between;
}

    if you added an align-items: center;, this would horizontally center the content (horizontally because of flex-direction: column;), and should achieve what you're trying to do with the various place-content: center; and text-align: center; rules. This rule on the flex parent (your main wrapper for each component) would horizontally center its children. Thus, you don't need the wrapper <div>'s around your <p> elements to center them. Writing less CSS/simpler HTML to achieve the same thing in this way will save you a lot of headaches in the long run, especially on larger-scale projects. Of course, sometimes wrapper <div>s are necessary for styling so you should still keep that tool, but in this case they aren't.

    Hope this helps!

    Marked as helpful
    1
  • geoffjecrois•390
    @geoffreyhachSubmitted over 2 years ago

    No particular issues, i learned the basic of the fetch method with and API. Still a lot to learn, the "then" method is still a bit obscur for me.

    Fetch method with an API

    1
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Nice job! You're using fetch just fine here. I found asynchronous JS pretty tricky for awhile. You might find Sid's answer to this StackExchange question informative, as it's a pretty comprehensive answer. His breakdown of the Promise API (steps 1-6) is especially helpful.

    You might also find async/await syntax more intuitive, which I use in my solution. It lets you, for example, assign the results of the asynchronous action to a variable in a "synchronous" looking manner. From my code:

    **async** function getAdvice() {
  const { id, advice, } = **await** fetch('https://api.adviceslip.com/advice')
    .then(res => res.json())
    .then(data => data.slip);
  ...
  ... (DOM stuff with my variables)
  ...

    (Note that I'm using object unpacking to get the id and advice properties from data.slip.) Essentially, I'm able to store the returned value of the last then statement into variables. Using await means that my code won't go forward until the asynchronous action is complete. Without the await, what's returned would just be a Promise, not an object with id and advice attributes, and I couldn't use those variables.

    To be clear, your solution is also completely correct/understandable. I just wanted to point out this new syntax since a lot of people find it easier to wrap their head around than the old methods of asynchronous programming. You might prefer Promise/then instead, and even if not, you'd still need to understand them a bit to use async/await.

    One last thing I'll talk about is that your has a couple pesky scrollbars, even though they aren't necessary. They're not a big deal in this case, but they could be annoying in your future projects. Here's how you can get rid of them:

    The vertical scrollbar

    You set your <main> to have height: 100vh;, presumably to center your card vertically. Then, when you add your <footer>, this is additional content, so that the total space is greater than 100vh, hence the vertical scrolling. Centering things vertically is pretty tricky, and I can't think of a simple solution here. Where I would start would be putting height: 100vh on the <body> instead of the <main> (actually min-height: 100vh;; using height: 100vh would be bad practice since it could mess up when content actually does overflow the screen vertically) and then do some sort of positioning to get your footer at the bottom. (Maybe you could align-self it with Flex?) This article gives some helpful information on centering things vertically, though it doesn't cover your exact use case.

    Anyway, that's where the vertical scroll bar is coming from.

    The horizontal scrollbar

    I believe this is because you're setting width: 100vw; on your main. I believe you're doing this to center the content horizontally, but I don't believe it should be necessary, as block elements (such as <div> and <p>) take up all the available horizontal space. I believe <main> is such an element, so it should already be taking up the width of the screen. I believe what's causing the horizontal scrollbar is that your vertical scrollbar from the previous point is adding on a bit of width to the page, so the width of your page is actually 100vw plus the width of the vertical scrollbar. That causes horizontal overflow.

    I think removing width: 100vw; should fix this. In the future, when you do want scrolling but don't want it to mess with the width, you could use something like overflow-y: overlay;. This will make it cover any content below it though, so keep that in mind.

    Hope this helps!

    Marked as helpful
    2
  • Ciceron•940
    @MarcusTuliusCiceronSubmitted over 2 years ago

    I couldn't find webkit property to make the past part of the slider change color, so it is working only for firefox. Does anyone know how to style this for webkit based browser ?

    It make me so mad i did not even complete the mobile style :D

    Sliding component using JS Sass and html

    1
    Grog the Frog•480
    @GregLyonsPosted over 2 years ago

    Ahh I gave up on this challenge because I got so frustrated trying to style the slider. I couldn't figure out how to get the left part filled in, while having the right-hand part be not filled in (with only HTML/CSS).

    Seeing this I remember that I actually had built a slider in a React project I worked on (so not pure HTML/CSS). I was following this tutorial for a double slider, and adapted it to make a single slider. Adapting that post to a single slider and in HTML/CSS/JS context, essentially, you have an HTML set-up like this:

    <div class="slider__wrapper">
  <input type="range" class="slider__thumb">
  <div class="slider">
    <div class="slider__track" />
    <div class="slider__range" id="range" />
  </div>
</div>

    Then you would use CSS to

    1. Remove the background from the <input>, so that only the thumb is visible (hence the class name slider__thumb);
    2. Use slider__track as your basic background (the gray, empty one), and slider__range as your filled-in background (the green one);
    3. Use position: relative; on slider__wrapper so that you can position: absolute; slider__thumb and slider to center them properly.

    Finally, you would use JS to manipulate the width of slider__range based on the value of the input. So if the input value is 1 and the max is 5, then slider__range should have width: 20%;. This makes it so that changing the value of the input changes the width of the background slider__range, which should achieve the desired effect. (Also, you may need to set some z-indexs to get it to look right.)

    It's pretty complicated, all to just to get the background of the slider to be two colors instead of one... I hope that all makes sense. I'd love to see a simpler/pure CSS solution though, as this challenge is marked as HTML/CSS only.

    Marked as helpful
    0