Skip to content
OverviewSolutions
13
Comments
6
Leon Pahole
@leonpahole

All comments

  • Leon Pahole•480
    @leonpahole

    Hey Koi, first of all, in a real world project, making decisions for how to display something on a certain screen size is not up to us developers, but up to designers. So whenever I have a dilemma like yours in a real project, I always step up to the designer or whoever else is responsible for designs. They will know what to do to make it look nicer, and then it's up to us developers to implement that. We call this a "product" or a "design" decision.

    It's really important that as developers we know what is in our realm of decisions and what we should reach out for. A good developer is not the one who can solve every problem, but the one who knows when and who to ask to get answers. This can save a lot of time on a real world project.

    Since in this case you don't have a designer you could ask, you have to kind of play it by ear. My suggestion is that you either reduce the image size or break into columns earlier.

    By the way, I suggest that you wrap the image in a div, because image as flex child can behave somewhat weirdly.

    I played around a bit with your site and did the following (my additions are in inline styles, but don't use inline styles):

    <div class="content" style="max-width: 1000px;">
<div style="max-width: 600px;display: flex;align-items: center; flex: 1;">
<img style="width: 100%; height: auto">
</div>
<div class="text" style="flex: 1" >{...}</div>
</div>

    And it seems to be looking quite nice. What I did here is I made both sides equal width, but the image container is capped at 600px. Also since image is in a wrapper, it is easier to make it preserve it's aspect ratio. Then I also capped the width of the wrapping container at 1000px. This prevents the text from expanding into a single line on really large screens. Of course you can tweak all of these numbers.

    Side note: I did not adjust for mobile using my styles. You will have to use media queries to make sure the site still looks good on mobile.

    Marked as helpful
  • Leon Pahole•480
    @leonpahole

    Hey Kailynne, nice solution :)

    Regarding your question about using too many div elements, to first give you the precise technical answer, yes, you can do it with less divs. For example, you don't need the div with the "content" class, as it kind of "double-wraps" the card.

    In addition, you could also remove the "text" div, but then you'd have to set max-width: 90% to both text nodes, so it would result in some code duplication - so I'd argue that while the "text" div could technically be removed, it is much better that it is there so you only set max-width: 90% in one place.

    Now, for a more "human" answer, the reality is that the amount of divs you use isn't really that relevant, as long as their usage makes sense to you as the developer. It is true that if you use a lot a lot a lot of elements on the page, the performance of the site will suffer, but here I am referring to huge amount of elements. So I wouldn't stress too much about the number of divs, and instead would focus on making sure that the code is understandable :)

    Some other comments about your code:

    • I noticed that you used font-family on every text node in the css. You can take advantage of CSS inheritance and set the font-family just in the body. Then all children will inherit the font-family. See details about inheritance here.
    • Try using relative units (like rems) instead of absolute units (px) for font sizes. More information in this blog post.
    • Since all of your colors are in CSS variables except for the color of the a element, I would suggest storing that color in a variable as well, to keep things consistent. Good idea on using CSS variables for colors!

    I hope this explanation helps you :)

    Marked as helpful
  • Leon Pahole•480
    @leonpahole

    Hey hey Stephen :) I wanted to answer your question regarding why the image was not square.

    First of all, if you set w-64 and h-64 (that is, same width and height) on the image, it should cause it to be square, unless some other declaration is preventing it from being square. In your case, if we set width and height of the image to be 16rem (that's what w-64 and h-64 do), you will notice, that the height will be equal to 16rem (256px), but the width will not be - it will only be 232px. So why is that?

    Feel free to inspect your styling and try to figure it out before checking the answer.

    So the reason is that your image has a max-width property, which is set to 100%. This means that regardless of what you set as the image width, it's cap (maximum) is 100%. But what is 100%? Well, 100% refers to the available width that it's container gives the element. The container is an article element, which has a padding of 0.75rem and a width of 16rem. This means that the image will get 16rem - 2*0.75rem (accounting for left and right padding) = 14.5 rem - and that's exactly 232px. Thus, the image can have a maximum width of 232px - if you set more, it will be capped at that value.

    Now, you could just remove the max-width property on the image, but that will stretch the image past the paddings of the container. Thus, to solve this, you will have to increase the width of the container.

    By the way, there is a way to have the image be a perfect square: by using aspect-ratio: 1 in the CSS. But beware that the property has (only) 90% browser support.

    However, because the image is naturally a square, there is something else you can do: you can set one of the dimensions and then set the other to auto. This will try to establish an original aspect ratio of the image, which in your case is a square. So what you can do is set the width of the image to 100%, and height to auto. Then set width on the article to whatever you want and you will see that the image will adapt to stay the square regardless of what the width is. This approach is great because if someone modifies the code later to change the width of the container, they only need to change one value (as opposed to changing both the width of the image and the width of the container). If the image wouldn't be a square originally, then as far as I know, the aspect ratio approach would be the only other way to achieve this without hardcoding the image dimensions.

    Let me know if my explanation made sense! And good job on completing the challenge! :)

    Marked as helpful
  • Leon Pahole•480
    @leonpahole

    Hello Dominik!

    I took a quick glance at the code and here are my comments for imrpovement:

    • Your font-family declaration in css has no fallback font. Check this blog post for why this is important and for more information.
    • Your css selectors are very broad, like targeting the article tag. If you happened to have two articles on the page, they would both be styled by this declaration, which may or may not be intended. I suggest you instead use classes to target your elements. See this blog post
    • The container has width of 350px. This means that it is not scalable. If you resize the screen to below 350px, you will see horizontal scroll bar appearing. Instead, it might be better to use max-width with width set to 100%.
    • The container has height set to 530px. Setting fixed heights is very dangerous, because if you later change the size of the image, font size, text contents of the container or if the user manually changes their font size, it will cause the content to overflow the container. Instead, it would be better to just not set the height and let the content and the paddings dictate the height of the element.
    • Some font sizes are specified in px - I suggest you specify them all in rem. This great blog post explains why.
    • The folder structure is a little unconventional. The README file is in the assets/md folder, so it is difficult to find. README should ideally sit in the root folder, so it is automatically displayed when you open the repo in Github (unless it pertains only to a specific folder). Also, built code should probably not be commited (by built I mean css that is compiled from sass).
    • It is not apparent on how to run the project, how to run the sass compilation, etc. I suggest you look into Webpack, which will streamline the development process and document it so that anyone can easily run the project with a single command, rather than having to manually compile sass.

    Other than that, the code looks good to me and the component looks close to the design.

    Marked as helpful
  • Leon Pahole•480
    @leonpahole

    Hey there, here are a couple of things I've noticed in your project:

    • The code is simple and clear.
    • The font-family is set as "system-ui, 'Outfit', sans-serif ...". The way how this works is that the first font that is available in the system for a specific glyph will be taken. This means that system-ui has priority over Outfit and in my case it renders instead of Outfit. If you'd like to have Outfit font whenever possible, you should swap these two. Read more here: https://linuxhint.com/css-font-fallbacks/
    • The page has a little bit of scroll even when there is enough space for the component to appear. Even though you set the main component to be 100vh of height, the padding is not accounted in. This behavior can be changed by changing the box-sizing property. Read more here: https://css-tricks.com/box-sizing/
    • Setting 100vh height can cause issues if the screen is resized to the height that is lower than the QR component (I suggest you try it yourself). It is usually recommended to set min-height instead of height so the content can expand past the height if need be.
    • You used height: fit-content, which is a relatively new property value. I suggest that you always check if browsers support the properties. You can use "Can I use": https://caniuse.com/?search=fit-content
    • The code doesn't appear to be formatted. I suggest that you use Prettier to format the code in a consistent manner. This can save you a lot of issues especially when working in a team and everyone has a different code style.
    • You have styles for .attribution both in the css file and style tag in the HTML. I suggest you unify these in one place, as scattered code that refers to the same thing can negatively impact readability.
    Marked as helpful
  • Leon Pahole•480
    @leonpahole

    I'm also getting an accessibility warning that I don't know how to solve: "All page content should be contained by landmarks" for the background image that is not part of the <main> tag. How to indicate that this image is just for decoration? I tried with alt="", but that doesn't help with the warning.