nft-preview-card

Community feedback

• 

  P

  Teodor Jenkler•3,740

  @TedJenklerPosted 2 months ago

  Hi @YuliaLantzberg,

  Nice project! I really like the overlay effect. However, I noticed that your site isn’t fully responsive. I recommend looking into the 850px - 998px and 1500px width ranges to ensure it displays correctly across different screen sizes.

  Image Positioning: Usually, it’s best to avoid setting fixed heights, but in this project, you should consider locking the image in place. You can achieve this by setting the card to position: relative and the image to position: absolute, then using top, left, and right properties to lock the image onto the card.

  Div Usage: Only use <div> elements when necessary to simplify your structure and make the code cleaner. You usually only need <div>s when you need content to change direction; the rest can often be achieved with a single flex column.

  Keep up the great work!

  Best, Teodor

  Marked as helpful

  1

  ---

  

  Yulia•340

  @YuliaLantzbergPosted 2 months ago

  Hi @TedJenkler. Thank you so much for the valuable and thoughtful feedback. I will definitely look into endpoints that you've mentioned and will try it with your recommendation about the image positioning. You are completely right about the divs. It's my weakness and I will try my best to improve myself and to reduce usage of the divs. Wishes, Yulia :-)

  1
• 

  T

  Grace•31,090

  @grace-snowPosted 2 months ago

  You've had some really good feedback above from @geomydas but it doesn't sound like you're following it. I wish I could add screenshots here to show you how this is very broken at lots of screen sizes and it is inaccessible.

  Despite discussions above, this challenge doesn't need a media query. All it needs is a max-width in rem on the card and a CSS reset at the start of the styles (which should be included in every project).

  But these are the kinds of things that are usually learnt and practiced in earlier challenges. The main focus of this specific challenge is almost entirely about accessibility and learning how to use pseudo-elements for the hover styles. (I've written up a full instructional post about how to implement that correctly in the Frontend Mentor discord resources channel if you want to head there and take a look -- I really must move it to a public blog!)

  This is all really important stuff and shows some foundational misunderstandings. If you repeat these issues moving into larger projects you're going to have a lot of unnecessary styling battles as well as building inaaccessible (which also means illegal) sites.

  I recommend you make the changes as suggested but also try to go back through earlier challenges and get the feedback on those. Because it's no good blindly applying changes if you don't understand why they're needed. If you're not already doing so, following the learning paths would be very beneficial.

  Marked as helpful

  0

  ---

  

  T

  Grace•31,090

  @grace-snowPosted 2 months ago

  To be clear on the above feedback, all of these matter:

  • Use a CSS reset
  • Consider using CSS Variables / Custom Properties. Not essential but small projects are still a great practice ground for it.
  • Use rem/em for media queries for accesibility (doesn't really matter if rem or em is choosed) - Agree, this improves layout significantly for people like me to need to change their text size.
  • Use max-width instead paired with width: 100% on the card rather than using media queries.
  • You may want to explore some CSS naming conventions (like BEM) to see if it helps you. Small challenges are a good time to try these out and see how they can make code clearer, more readable, maintainable etc.
  • Setting font size on the body is fine as long as it's in rem (@geomydas I disagree with your recommendation there)
  • Use min-height: 100vh instead of height: 100vh for centering something to the viewport because using height will not make your content grow and it may cause content to be cut off at smaller screen sizes
  • See about CSS pseudo-classes such as :hover, :focus, :focus-within, :active and etc. - You definitely need this
  • Remove position: relative on the body as it does nothing. Relative is typically used if you have a child that is absolutely positioned or you want to increase the z-index of that element which is not the case here
  • Use a code formatter such as Prettier for better code readability
  • Learn how to write better alt text for images (important)
  • Don't set heights on elements that often since the content of that element (text, paddings, margins, images, etc.) will most often be enough to take the space. Use min-height instead in fixed units
  • Don't use the 62.5% font size hack. It's bad practice

  ⚠️ REALLY important is your solution is really really really broken because of all the widths and heights in percentages. Remove them all. All this component needs is max-width in rem and a css reset. The little icons and the avatar image need an explicit width and height but nothing else. Width 100% will be used in a few places.

  Marked as helpful

  0
• 

  geomydas•1,400

  @geomydasPosted 2 months ago

  • Use a CSS reset
  • Use more CSS Variables / Custom Properties for your colors
  • Use rem/em for media queries for accesibility (doesn't really matter if rem or em is choosed)
  • Use max-width instead paired with width: 100% on the card rather than using media queries
  • Use BEM naming methodology for naming classes for better maintainability and readability
  • Don't set a font-size on the body since you will likely just override it with your font-sizes on the element
  • Use min-height: 100vh instead of height: 100vh for centering something to the viewport because using height will not make your content grow and it may cause content to be cut off at smaller screen sizes
  • See about CSS pseudo-classes such as :hover, :focus, :focus-within, :active and etc.
  • Remove position: relative on the body as it does nothing. Relative is typically used if you have a child that is absolutely positioned or you want to increase the z-index of that element which is not the case here
  • Use a code formatter such as Prettier for better code readability
  • Learn how to write better alt text for images
  • Don't set heights on elements that often since the content of that element (text, paddings, margins, images, etc.) will most often be enough to take the space. Use min-height instead in fixed units
  • Don't use the 62.5% font size hack. It's bad practice. You can just use calc(pxUnitrem / 16) or Sass functions. See why

  That's a quick summary and as a reminder make sure to mark my comment as helpful if it is indeed helpful

  Marked as helpful

  0

  ---

  

  Yulia•340

  @YuliaLantzbergPosted 2 months ago

  @geomydas

  • I've used the basic reset. There is no need to reset elements that I'm not going to use.
  • All the point of variables that I can reuse them. As long as other colors I used only once in a code there is no point in variable.
  • Not really. Check this article https://www.davebitter.com/quick-bits/rem-unit-media-queries
  • When BEM indeed is a comfortable and readable naming system, I think it is redundant for a small component with small amount of nested elements and even can be overwhelming
  • To define or not font-size in body is really depends on the design. And most likely it will be good for keeping code dry, without unnecessary repetitions. It also improves maintenance, as if needed you can change it in one place, instead of several elements.
  • In this case, I didn't want the container to grow, I wanted it always to be in the boundaries of the viewport.
  • I've used :hover pseudoclass here. What is the need to add focus or active in this component?
  • Position relative in body is used to align the footer element with position absolute and bottom properties.
  • I do use Prettier.
  • Well here couldn't find out how I can describe better the image.
  • Again, not always I want the unpredictable growth of the height. In this case, looking at design, I think that fixed height is a better choice.
  • Indeed, it's a good and convincing article. Since now I will avoid the 62.5% font-size hack. However, here is the counterargument about the matter: https://adrianroselli.com/2024/03/the-ultimate-ideal-bestest-base-font-size-that-everyone-is-keeping-a-secret-especially-chet.html#comment-285002 Thx

  0

  

  geomydas•1,400

  @geomydasPosted 2 months ago

  @YuliaLantzberg For larger projects, consider using one. It's just in case that if your project grows anyways. Most projects scale over time so thats why its good practice to use CSS variables even if you think you are using them once.

  The article that you sent to me is just a warning that rem in media queries won't work if you change the base font size inside the HTML which is an accesibility issue. Rem over px is still best practice inside media queries. For example

  @media (min-width: 1080px) {
    body {
      background-color: red;
    }
  }
  @media (min-width: 67.5rem) {
      body {
          background-color: blue;
      }
  }
  

  Both of them will stay the same assuming I don't do some shenanigans in the html font size. But if I increase my font size in the browsers setting, the px one will activate first. If I decrease my font size, the rem one will activate first

  I'm not reccomending you to use other pseudo-classes such as :focus or :active but keep them in mind in future projects.

  I will take a look at setting the base font size to 62.5% as it seems there is a counter argument for it.

  What do you mean by the "unpredictable growth of height". It won't grow out of nowhere unless some text wraps and it is normal.

  0

  

  Yulia•340

  @YuliaLantzbergPosted 2 months ago

  @geomydas

  • This is a good argument and indeed use it when writing code in programming languages. But here I take it as a learning project. There are some things that very useful and comfortable in big projects and less suitable for small ones. There is a good practice to break project for small components. But still we don't do it in a small project, because it feels overwhelming and makes reading harder than easier.
  • This article says that using rem in media queries can give unpredictable results as rems in your code will be calculated accordingly to the users or developers font-size choice but media-queries rem will be calculated by 16px default font-size of the browser, regardless the choice of the user. Therefore, using rems in media-queries is not good not for design nor for accessibility.
  • When I say "unpredictable growth of height" I mean the changes in the content. Sometimes I don't want that height will be dependent on the content's size changes. Here is a discussion about it https://stackoverflow.com/questions/2400985/is-it-better-to-use-min-height-always-in-place-of-height-in-css

  0

  

  P

  Teodor Jenkler•3,740

  @TedJenklerPosted 2 months ago

  @geomydas You don't want to use rem in media queries. rem units inherit their value from the base font size, while media queries should use fixed values. A 1440px width laptop should always be 1440px. If you use rem and change the font size, it could cause the media query to not correspond to 1440px accurately.

  Marked as helpful

  1

  

  T

  Grace•31,090

  @grace-snowPosted 2 months ago

  Sorry @TedJenkler, but that's just not true. Media queries should be in rem or em ideally so that layouts do adjust when users have a different text size.

  For example, if I have my text size at 200% (32px) I want the layout to adjust. I don't want a desktop layout to remain with massive text. I've written about media queries here https://fedmentor.dev/posts/responsive-meaning/#how-to-best-approach-responsive-styling

  It's not a failure to write them in px, but from many years of dev experience, and as someone who does change their text size often, I can confidently say media queries in rem or em results in a much more responsive and accessible site.

  I'll add that a few years ago there was a massive influx of blog posts arguing about this which was a big waste of time. Most of those posts had misunderstandings about how the browsers calculate (which have also changed in the last 2 years because there were some outstanding bugs) or they had almost no awareness of the accessibility impacts. (e.g. the linked article above includes an example where the html size is set in PIXELS!) They were arguing over whether media queries kicked in at a few px difference when units where compared to each other, which doesn't matter at all. That shows they're not following modern intrinsic design principles. (You don't need media queries to change at a specific pixel - you need changes to happen whenever they need to change due to content or space constraints or the end users environment and settings).

  0

  

  P

  Teodor Jenkler•3,740

  @TedJenklerPosted 2 months ago

  @grace-snow This is interesting—I wasn’t aware of the accessibility aspect. But why would you use rem in media queries when it could unintentionally break the layout for some users? rem already makes the site responsive when used for text and so, and using it in media queries seems to add unnecessary complexity. It feels like using em or rem in media queries could make the site unpredictable for different users who might have different settings. In the worst case, you could just use rem for setting text size with clamp.

  I'm curious, do you use this as a standard at your workplace, or are you a freelancer/individual developer? I’ve never seen rem or em used in media queries, except maybe with the new container queries, which work similarly. I’m not sure how it could be applied in a professional setting(using it randomly would just be bad code so either us it or not use it and stick to it).I don’t really see the advantages of using rem or em in media queries because precision is usually the most important factor. I haven’t seen anyone use rem or em for this purpose before.

  I think I’ll comment and see if Kevin Powell has an answer, because I’m far from an expert, but I just can’t see the reason for using them in media queries when you want to target specific devices.

  Thanks for the answer interesting topic

  1

  

  T

  Grace•31,090

  @grace-snowPosted 2 months ago

  @TedJenkler I am a senior developer and accessibility specialist with many years of experience working all for other agencies or companies who always used em or rem for media queries in most projects. The only exceptions were older library based projects like bootstrap ones. Using media queries in rem or em will not break layouts if they've been built following modern intrinsic layout techniques and I've never seen them cause a problem unless there is poor quality code with heights and widths everywhere. So no idea what you mean by it would break layouts.

  Really though this is a waste of time discussion. It's not worth the mental energy.

  Change font size in browser and see what happens.

  0

  

  P

  Teodor Jenkler•3,740

  @TedJenklerPosted 2 months ago

  @grace-snow Well, if it's used professionally, then I'm clearly wrong. I just want to add that your attitude is really poor for a senior, and your comment seems more ego-inflated than an attempt to be a mentor, but maybe it's just a bad day for you.

  Anyways, have a nice day.

  1

  

  T

  Grace•31,090

  @grace-snowPosted 2 months ago

  @TedJenkler I'm not having a go. I'm just saying people get all hung up on discussing these tiny things endlessly and it doesn't matter. There are so many more important things. When I say it's a waste of time that's coming from a place of being drawn into a bit tangent that isn't actually relevant. This is a feedback space on a challenge that doesn't have any relevance to the topic. So pause, down tools and don't get drawn into these long winding discussions about a side topic — just try it, play with the tools including changing settings like text size, and come to a conclusion about your preferences.

  0

  

  P

  Teodor Jenkler•3,740

  @TedJenklerPosted 2 months ago

  @grace-snow I understand that’s your view, but it’s important to recognize that discussions on topics you might not find interesting are still valid. I would suggest reviewing some of your word choices, as they can come across as negatively charged. That might not have been your intent—it's hard to tell, as this is the first time I've seen you.

  Anyway, I looked into what you mentioned and realized that the Safari compliance issue is no longer a problem. However, I still prefer using px, as it feels more intuitive when reflecting actual screen sizes.

  For example, most people are familiar with the approximate size of a 1440px screen or a 375px phone. But when they see a media query with a breakpoint like 37.5rem (just an example), converting from px can feel unnecessary. In my opinion, readability declines, and even with tools like ChatGPT or browser extensions, the time spent converting px to rem doesn’t seem worth it.

  0

  

  geomydas•1,400

  @geomydasPosted 2 months ago

  @TedJenkler If you're using Sass, you can use custom functions to convert rem to px. It will output as rem in the final css file but it can look like px in the .scss file. But maybe there is an argument for complicating stuff using functions

  @media (min-width: toRem(160px))
  

  1
• 

  Yulia•340

  @YuliaLantzbergPosted 2 months ago

  So taking in consideration all the discussions above, I've rewritten the entire project, HTML and CSS both.

  1