This was a good project! I had a hard time figuring out how to change the input value if null so I could add the class onto the element displaying the text Not Available. Getting this one part right took 75% of the time I spent on the challenge : ). In the end, I finally figured out that you can use the || operator in template literals. The only link that I couldn't get to work was for the user's company, I'm not sure why. If anyone can figure it out let me know!! I also made it so that it brings up my GitHub profile on load so if you want to compare the design to the one I made go to the live site and search octocat.
Jason Moody
@MoodyJWAll comments
- @EileenpkSubmitted over 2 years ago@MoodyJWPosted over 2 years ago
Hi Eileen, great job on the challenge! Your question about the company, I think I see what's wrong.
You're using the
company
property on the user data, but the one you want is actuallyorganizations_url
. A bit confusing, but if you look at the data in your network tab you'll seecompany: null
and that property is actually just astring
for the user's company,company: "Company Name"
.A styling issue I noticed is that you haven't accounted for longer strings overflowing their container. Here's a screenshot of what I'm talking about. You can probably solve this pretty easily by adding a couple of styles to your
<a>
:text-overflow: ellipsis; overflow: hidden;
That would likely solve it for all screen sizes.
I also encourage you to take a look at your HTML report as you have quite a few issues there. For example, you typo'd on a
<label>
element.Hope this helps you out. Don't forget to be proud of all the things you did right!
0 - @mykexieSubmitted over 2 years ago
Did things in the code I'm not sure are legal. Someone please glance through my code... Any suggestions on how I can improve are welcome!
@MoodyJWPosted over 2 years agoHey Michael! You've done a great job on this. Here's some feedback.
-
Your code isn't formatted very well, especially the HTML
-
Generally there isn't much of a need for empty lines in HTML. Sometimes it's useful to break up large files into smaller pieces, but large projects are going to do this using separate files
-
The indentation is also significantly wider than it needs to be. Two spaces seems to be the most frequently used indentation for HTML
-
Your CSS is better, but still has some unneeded empty lines
-
You should remove comments. Doesn't really matter, but it's a good practice to only leave comments that are necessary to explain why some code is there.
-
You've used the
.img-container
class redundantly inside of the media query -
The responsiveness to screen sizes doesn't quite work on smaller screens. You might have more luck using a flex box design instead of a grid.
But the laptop screen is great! Hope this helps you out, keep at it
Marked as helpful0 -
- @12KentosSubmitted over 2 years ago
Definitely has some minor bugs, but overall works, took a break last week from coding, and would like to move onto a new project. Will probably come back at some point to finish fleshing it out. I also forgot to make it responsive to mobile... oops.
@MoodyJWPosted over 2 years agoHi Kent! Excellent job on the challenge. I noticed a few issues that might help you improve.
- definitely take a look at the HTML report, you have a number of accessibility issues regarding missing landmark elements.
- you have a styling issue in the links section at the bottom, here's a screenshot
When the text is longer as seen in the image, it's going to force the other elements to the right and it gets a bit sloppy. However, this is pretty easy to fix. There are a lot of ways to handle long strings, but my favorite is to just truncate and add an ellipsis.
On the container
div
, the links wrapper, addoverflow: hidden
to your sass. Then on thep
element, addoverflow: hidden; width: 100%; text-overflow: ellipsis
. That's it!The icons are also a bit clipped, but that's also easy enough to fix! If you open the
svg
file for the icon, there should be a width and height. Simply set the same width and height on the containerdiv
and it should be perfect.Great job, keep it up!
Marked as helpful1 - @HamnaIshaqSubmitted over 2 years ago
Hi everyone!
If you have any suggestions/ tips, please feel free to comment.
@MoodyJWPosted over 2 years agoThis is an extremely well done project! I don't see anything that stands out in the code. Couple of styling issues, though. On a laptop screen (1440), the card itself seems a little shorter than the design. Not a very big problem at all.
The other is it looks like you didn't implement the background image. Generally the easiest way to apply a background image is to add the below styles to a container element or to the body directly.
background-image: url(./image/blahblahblah); background: var(--paleBlue); background-size: cover;
You might need to adjust based on the way your HTML elements are structured, but there are a lot of options for background images so it's usually best to play around with it in the browser dev tools. The (MDN for background)[https://developer.mozilla.org/en-US/docs/Web/CSS/background] is a great source, it should break down all of the property options.
Hope this helps you out. Great job on the challenge, keep it up!
Marked as helpful1 - @tssantosSubmitted over 2 years ago@MoodyJWPosted over 2 years ago
Hi Thiago! Great job on the challenge! I noticed a styling issue in the section with location, twitter, etc. If the content is longer it will overflow the container and cover the second column.
Here's a screenshot of what I'm talking about
You can probably fix this with some relatively simple CSS on the element containing the text
text-overflow: ellipsis; overflow: hidden; width: 100%;
As long as the parent element has a width, the text should cut off at the edge of the parent element. You might need to adjust that width based on how you've set up the flex boxes.
I'd also suggest trying to start developing for mobile first, it makes styles a lot easier to adjust when you're getting bigger instead of smaller. Plus over 90% of people in the world access the internet via mobile, so most frontend work is going to require responsive styles. Still, this looks really good and the small change to the text should make the desktop version perfect! Hope this helps, let me know if you need more info.
Marked as helpful0 - @sohhammSubmitted over 2 years ago@MoodyJWPosted over 2 years ago
Hello @ sohhamm! You did great on the challenge! I noticed one issue with the styling of the bottom section with links to Twitter, location, etc. If the displayed user has a longer url the section stretches to fit the length and forces some of the content to the edge of the card.
Here is a screenshot of what I'm seeing
You can solve this in a few ways. I generally prefer to truncate the text and use an ellipsis to indicate the text has been cut off. A tooltip is a great way to display all of the text in these cases. The easiest way I've found to do this is with CSS on the element containing the text. As long as there is a container, this should pretty much always work.
white-space: nowrap; overflow: hidden; text-overflow: ellipsis;
You also have a second problem in this same area, which is how your grid is designed. You've used
grid-template-columns: 1fr 1fr
, which is good if the columns have the same size content or if you don't care about stretching/shrinking grid cells as needed. However, what's happening in this case is the first column is as wide as the contents and the second column takes up the remaining space or the same space as the first column, whichever is smaller.There are a lot of ways to solve this, but here's a simple way I did it without significant changes to your code.
.extraInfos { ...existingStyles; grid-template-columns: 50% 50%; gap: 18px 64px; }
So we change the columns to both be
50%
, forcing them to maintain their width and not stretch.flex { ...existingStyles; align-items: center; }
Centering the content in the flex box removes the vertical stretching on the icons
.extraInfoTitle { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; }
And this is the change mentioned earlier to truncate the text.
Here's a screenshot of what it looks like after I made these changes in the browser.
So not really that big of an issue, just a few small changes! This project gave me a headache on that exact same part. Hopefully this will help you out!
1 - @Hazel-BlackSubmitted over 2 years ago
Wow, this was a great experience. I learned so much about responsive design and I'm so happy with how everything turned out. any feedback is welcome, did I use too many divs. could I have formatted my CSS better? let me know what you think below
@MoodyJWPosted over 2 years agoHi Hazel,
Great job on the challenge!
Some styling suggestions:
- the
border-radius
on the hero image isn't correct, the bottom is rounded when it shouldn't be - you could add
hover
states to the other buttons - most of the time it's not an issue, but generally it's good practice to add a generic
font-family
such as serif, monospace, etc. This acts as a fallback in case the font isn't available. Here's more info - look into using a CSS extension like Sass
- your CSS is a bit disorganized, which can be a hassle for people doing code review. I suggest using a formatter add-on, if you're using VSCode there's one called Prettier that's used by a lot of people
HTML suggestions:
- moving the footer to the bottom of the page would be ideal
- watch out for accessibility issues
- using HTML elements like
article
,header
,h1
, etc is a good idea because it will help people using screen readers navigate through the page. Here's a resource on it - I suggest using a
button
element for things like thecancel order
action instead of just ap
Overall you still did a great job and these suggestions won't really change much about the look of the product, but they'll make your code more accessible and legible for others. Also slick move navigating to your YT from the change anchor!
Speaking of YT, Kevin Powell is a great resource for CSS/SCSS videos.
Marked as helpful0 - the
- @arnavdoleySubmitted about 3 years ago
this is my first project would love to hear suggestions to improve it
@MoodyJWPosted about 3 years agoHi Arnav, great job on your first challenge!
I'd suggest taking a look at your report as you have some HTML issues.
-
heading tags like
<h1>
,<h2>
, etc should always appear in order. The reason for this is that it allows people using screen readers to more easily navigate the page. If you need to adjust the size of the heading, just use CSS -
after your
<body>
, you should wrap content in a<main>
tag. This is also mostly for accessibility reasons. You could swap your<div class="container">
-
<img>
tags can only have widths in pixels when done inline, you can generally solve this by using a CSS class -
on the page background image, you would probably have an easier time adding that image on the
body
in your CSS. Something like below would do the trick, but you might need to adjust if your other CSS interacts
background: var(--pale-blue) url("../images/pattern-background-desktop.svg"); -webkit-background-size: cover; -moz-background-size: cover; -o-background-size: cover; background-size: cover;
- another suggestion is to clean up your code a bit. Generally it's a good practice to avoid empty lines within your CSS class or randomly in your HTML. Clean, easily read code will help you get a job if you're trying to and it'll make people more likely to review your code here and elsewhere.
You can use a code formatter if you code in an IDE like VSCode. A lot of people use one called Prettier, but there are many others. I'd also suggest looking into learning Sass, it's a CSS extension that has a lot of neat features; it's also a "superset" of CSS, which means any valid CSS will work in a Sass file. Kevin Powell has a YouTube video on how to get started with Sass that I found helpful.
Anyway, that's probably enough for now. Good luck, hope some of this helps!
Marked as helpful0 -
- @rohan843Submitted about 3 years ago
Any feedback will be very helpful
@MoodyJWPosted about 3 years agoHey there! Check out your report for some HTML issues, looks like you need to add a
<main>
landmark. This link is helpful for understanding how/why. You also used backslashes instead of forward slashes in a few places. I'm not certain if you can use%5C
for a backslash here or if you just want to use/
instead, but there is more info hereYou can also use an HTML validator to test for these kinds of errors when you're not using FE Mentor.
0 - @Victor-NyagudiSubmitted about 3 years ago
I had some trouble when it came to changing the image color from black and white to the purple used in the design.
I'm aware of the
filter
property that can be used on images, but I'm not sure how to use it to apply a purple filter. Does anyone know how to do that, or should I just add the filter using some photo editing software?Also, while making the mobile version, I felt like making it without up/down scrolling would've resulted in really small fonts, so I kept the larger font with some scrolling.
Did anyone else follow a similar approach?
@MoodyJWPosted about 3 years agoHey there! I was having a similar issue on this challenge and never quite figured it out until I was working on something else more recently.
.container { width: 100vw; height: 100vh; background-color: rgba(255, 255, 255, 1); }
Above code sets the background color (obvi use your own values here), the 4th rbga value is the opacity.
.container::before { content: ""; background-image: url(your/file/here); background-size: cover; background-repeat: no-repeat; position: absolute; top: 0px; right: 0px; bottom: 0px; left: 0px; opacity: 0.1; }
Above code uses the pseudo-element ::before, which allows you to place the image with an opacity. Other props are for centering and covering the containing element with the image (change as needed).
.container > * { position: relative; }
Above will apply a relative position to everything below the
container
, without this other elements would also have the color filter.I tried working with a
linear-gradient
and/orblend-mode
, but had little success (I was probably just doing it wrong) and this worked well for me on my other project.0 - @beslerpatrykSubmitted about 3 years ago
Hello everyone 👋
This is my "Stats Preview Card Component" solution using pure HTML and CSS. I used mostly flexbox for layout but there is also some CSS grid there. I had a lot of issues with getting the proper hue of the image but in the end, I got a satisfying result. How do you guys usually approach the problem of positioning in small projects like these? At what elements do you use responsive units and on which you use definite values?
As always if you see any issues with this project - let me know. Any criticism/comments can certainly help me learn and grow as a front-end developer. Thank you 😁
@MoodyJWPosted about 3 years agoAs the others have said, good job on this challenge!
A quick tip on the positioning of the text. Your design isn't quite centered, which you can do by adding
align-items: center
on the.card_content
. Thejustify-content
property you have on there now only centers the content on the main axis. With aflex-direction: column
the main axis is a line from the top to the bottom;justify-content
will only center along that line.Hope this helps. You did great, keep it up!
Marked as helpful1 - @meenalhSubmitted about 3 years ago
This is my very first front-end challenge. Any feedback will help me improve, so hopefully, you'll let me know.
@MoodyJWPosted about 3 years agoHello! You did a great job on this. I noticed a few things, so I'll list out some pointers that might help you out in the future. A lot of the points below are accessibility issues, which aren't so important for making things look the way you want, but are incredibly important for people using screen readers.
-
on your
body
you've set the margins using each side like thismargin-right: 0px
,margin-bottom: 0px
, etc. You can simply domargin: 0
and it will set all sides to 0 pixels. You can also shorthand tomargin: 0 5px 0 5px
where each value is a side, starting at the top and moving clockwise. Or evenmargin: 0 5px
where the first value is top and bottom, the second is for both sides. -
an html standard is to wrap content in a
<main>
tag. I'd suggest putting it around everything inside the<body>
-
you don't want to use headings out of order, so always start with
<h1>
and work your way up within each section and avoid using heading tags for anything that isn't an actual heading. So your<h3>
forOrder Summary
is good, but it should be an<h1>
and the<h5>
you've used on the cancel action should honestly just be a button. -
like I mentioned before, you generally want to use buttons if clicking the element completes an action (think cancel, submit, save, etc)
-
your code is a bit sloppy, which is understandable because html and CSS are nightmares to format properly. However, there's an easy solution! If you're using Visual Studio Code, I'd suggest an extension like Prettier or other formatters. You can set them up to automatically format everything when you save
-
you're adding the font to a few different classes, but if you put it on the
body
you won't need to do it again unless you want to override with a different font
That's all for now! Hope this helps you out.
Marked as helpful1 -