Looking to hire developers?
@byronbyron
@DanK1368
Submitted
Hi guys, Since I'm practicing React as my first framework and recently familiarized myself with styled components I wanted to put that into practice with this challenge.
I'm hoping to hear some feedback on what I could have improved on/written more efficiently. :)
@byronbyron
Posted
@DanK1368 Hia mate.
Try position: fixed; instead of position: absolute;, to fix the scrolling issue.
@media screen and (max-width: 670px) {
.ggprRt {
/* position: absolute; */
position: fixed;
top: 0px;
left: 0px;
height: 100vh;
background-color: rgba(0, 0, 0, 0.5);
transform: translateX(110%);
opacity: 0;
}
}
Marked as helpful
@Chillidot
Submitted
Why is svg/image at top left corner not shown correctly in Safari-Browser?
@byronbyron
Posted
Hi @Chillidot
If you remove the height: auto; from the your style.css below, that should fix it in safari!
/** Circle **/
svg{
max-width: 100%;
/* height: auto; */
}
Hope that helps!
Marked as helpful
@ujwalvinay
Submitted
How to make the eye icon visible while hovering over the image? How to make it responsive for smaller devices?
@byronbyron
Posted
Hi @ujwalvinay
You can put the overlay hover on the image by adding an image wrapper and using a pseudo element, something like:
<div class="img-wrapper">
<img src="images/image-equilibrium.jpg" alt="image" id="banner">
</div>
.img-wrapper {
position: relative;
}
.img-wrapper::before {
content: '';
background: url('../images/icon-view.svg') center no-repeat hsla(178, 100%, 50%, 0.5);
display: block;
border-radius: 1.2rem;
position: absolute;
top: 0;
right: 5%;
bottom: 0;
left: 5%;
z-index: 100;
visibility: hidden;
opacity: 0;
transition: visibility 0.3s, opacity 0.3s;
}
.img-wrapper:hover::before {
visibility: visible;
opacity: 1;
}
Hope that helps! 👍
Marked as helpful
@MikeAngel2
Submitted
2nd card (Thanks Card) cannot center elements properly, can someone help me with that?
@byronbyron
Posted
Hi @MikeAngel2
You should be able to center the second card by updating your index.js to set display flex instead of block.
$(".Thanks").css('display','flex');
This bit of CSS should help to center the text on the second card as well
.Thanks {
align-items: center;
text-align: center;
}
Hope that helps!
Marked as helpful
@Abrosss
Submitted
Hello. I am not sure if height and width of the container should be dynamic, depending on the random advice text or fixed?
@byronbyron
Posted
@Abrosss Looks good!
Your question is probably one for whoever designed it. I went with a fixed width, but I don't think it matters too much.
Few html/accessibility issues:
Your <section class="page"></section> element is probably better off being a <main class="page"></main> (Document should have one main landmark)
Also need a <h1> element somewhere inside your <section class="container"> (Page should contain a level-one heading)
Marked as helpful
@Ganesh-Rathor
Submitted
Nice challenge for me.
@byronbyron
Posted
@Ganesh-Rathor To style range input on chrome/brave (webkit) you can use:
input[type=range]::-webkit-slider-runnable-track {}
input[type=range]::-webkit-slider-thumb {}
Marked as helpful
@jillpandya2594
Submitted
I have used grid-template-areas but now its not working.
@byronbyron
Posted
Hey @jillpandya2594, Hopefully this fixes your grid!
.container {
margin: auto;
max-width: 40rem;
padding: 2em;
display: grid;
grid-template-rows: 1fr 1fr;
grid-template-columns: 1fr 1fr;
}
main {
padding: 2em;
background-color: #fff;
grid-column: span 2;
align-self: flex-end;
}
section {
display: flex;
flex-direction: column;
padding: 2em;
background-color: hsl(179, 62%, 43%);
}
aside {
padding: 2em;
background-color: hsl(180, 62%, 47%);
line-height: 1.6;
}
@NellyisDevv
Submitted
This first project was straight forward and pretty fun to complete overall. The only issue I ran into the first time was making it mobile friendly, after I switched to flex box it, that issue was solved.
Centering my QR code to the center of my container was something I feel I could have done better, instead of doing this inside of CSS I opted for the <center> tag in html.
Is there any guides on how professional web developers set up their CSS files?
@byronbyron
Posted
Nice job @NellyisDevv
I would avoid using the <center> tag as it's well out of date, and no longer recommended.
To display your QR code a tiny bit better on really small screen widths (around 320px) I would do something like this:
.qr-container-background {
background-color: var(--White-);
padding: 20px;
border-radius: 20px;
max-width: 320px; /* Set max width of card */
width: 100%; /* Full width up until 320px */
}
.qr-container-photo img {
border-radius: 10px;
width: 100%; /* Set image to fill width of the container */
height: auto;
}
For a small project like this, I think your CSS file set up is absolutely fine!
Also, you've got a few Accessibility/Html issues that you could resolve for a perfect score.
Marked as helpful
@TotallySly
Submitted
I know the colour on the hover states is incorrect. But I just could not figure out the correct colour scheme to use.
Also, I am a bit concerned that the sizing of the design. It is massively different to the design brief. In the real world, would this be a major concern? If so, are there any websites that can scale images and give the correct pixels for my future design???
Again, thank you for all that will review my code and give feedback.
@byronbyron
Posted
Hey @TotallySly, it looks alright!
For the button hover, I used a slight opacity instead of the background colour, something like:
.payment--container button:hover,
.payment--container button:focus {
opacity: 0.75;
}
For the sizing thing, it depends on how picky your client is in the real world 😅. When I want to get the image size from the design image, I usually open it up in Preview (mac) and drag a square around the image to get the dimensions.
For your Order Component, it looks like your background images aren't coming through. Something like this should help I think (feel free to play around with the background-size values):
body {
...
background: url(images/pattern-background-mobile.svg) no-repeat, top center hsl(225, 100%, 94%);
background-size: 100% 194px;
}
@media screen and (min-width: 908px) {
body {
background-image: url(images/pattern-background-desktop.svg);
background-size: contain;
}
Marked as helpful
@EmmanuelOloke
Submitted
Used this project to test my bootstrap and CSS knowledge. Getting better every day.
As always, any feedback whatsoever is welcomed. Thanks.
@byronbyron
Posted
@EmmanuelOloke Looks good!
Your <section> elements ideally need a heading tag inside them somewhere 👍
That's my 5 pence 😅
Marked as helpful
@DanK1368
Submitted
Hi guys,
Please provide some feedback. :)
The app isnt perfect, I still have some problems to fix:
background-image: url("../../../assets/icon-check.svg"); This is my file path for the check mark inside the button. However, github pages isnt displaying it. Isnt that the relative file path to be used if the file is 3 layers deep?
The draggable feature works on the todo items that are already there. However, the code breaks once new todos are added? What am I doing wrong?
Thanks guys :)
@byronbyron
Posted
@DanK1368 Looks good!
background-image: url(../../assets/icon-check.svg); will get you the checkbox!Marked as helpful
@AdamMzkr
Submitted
Hi, I come back from a 1year break to the front-end and I try to remind important topics. I hope with the realization of the front-end mentor challenge my skills come back and even grow.
I am ready for critics :)
@byronbyron
Posted
@AdamMzkr Looks great!
Just missing a <main> tag, that's all.
Nice job 👍
Marked as helpful
@jackzorola10
Submitted
I'm wondering what is a best practice, for example I kept the <img>, <h1> & <p> with no classes, instead, since this is a very simple component I simply added .card and then the html tags.
Additionally on the report I get the following accessibility issues: "should be contained by landmarks", could you please explain me what does it mean by that? I have no clue on how to resolve that.
Thanks in advance!
@byronbyron
Posted
Looks good @jackzorola10
The landmarks issue basically means you're missing a <main> tag. Try wrapping everything inside the <body> with a <main> tag.
Hope that solves your issue 👍
Marked as helpful
@dreamspice
Submitted
Fully responsive with working switch :D
I tried use bem naming but i don't understand this concepction so - my class name is bad.
Good night guys.
@byronbyron
Posted
Hey @dreamspice your BEM classes don’t look too bad at all! The main point is that you’ve picked a convention and stuck to it. 👍
Marked as helpful
@dinesh671
Submitted
@byronbyron
Posted
Nice work, @dinesh671
@Christ-Kevin
Submitted
@byronbyron
Posted
Not bad @Christ-Kevin
I'd suggest having a look at the accessibility/html issues flagged by the FE mentor report 👍
auto for attribute height on element img: Expected a digit but saw auto instead.h2-h6 elements to add identifying headings to all sections.Other than that looks good! The massive height is a new one as well 😆
@Reandyx
Submitted
If u like my work leave a feedback!:)
@byronbyron
Posted
Hi @Reandyx
Looks really good!
Few accessibility issues flagged the FE Mentor report:
Both should be fairly easy to fix by making sure your heading start at <h1> and continue in order. The second issue can be fixed by wrapping all the content in a <main> tag.
Your transitions when the theme switches are brilliant!
@Ashraf-7
Submitted
@byronbyron
Posted
Hi @Ashraf-7
Looks like you've got a few issues flagged by FE Mentor's report thingy:
Other than those, it looks good 👍
Marked as helpful
@tadeubdev
Submitted
@byronbyron
Posted
Hi @tadeubdev
Your footer links look like they’re off centre slightly to the right on mobile.
Also a few accessibly/html issues flagged by FE Mentor.
Really nice work 👍
@naomy19
Submitted
Hi! I did this challenge to learn to make responsive sites, do you guys have any tips to make it better? I would love to receive your feedbacks :)
@byronbyron
Posted
Hi @naomy19
It looks very nice!
Couple of small suggestions as per the issues FE Mentor has flagged:
<main> element wrapping the content as well.Great work 👏
Marked as helpful
@saraferreira10
Submitted
This is my first challenge here. Feedbacks are welcome; thanks!
@byronbyron
Posted
Hi @saraferreira10
Looks great!
The frontend mentor report thingy is flagging a couple of issues, both I think can be easily fixed by using a <main> tag. e.g.
<main class="container">
...
</main>
Nice work! 👍
Marked as helpful
@jonathan401
Submitted
when i try to view the cards on a mobile device, the first card is not completely shown but when i try adding a height of 100% to the card wrapper everything seems okay except that the rounded corners on the card wrapper disappears i hope you could give me insight on how to solve this.
@byronbyron
Posted
Hi @jonathan401
Try this for the body:
body {
min-height: 100vh; /* min-height instead of height */
padding: 88px 1rem; /* a bit of spacing above and below for mobile */
font-family: sans-serif;
font-size: 15px;
display: flex;
justify-content: center;
align-items: center;
}
for the rounded corners on mobile:
.card-wrapper {
display: flex;
justify-content: center;
align-items: center;
flex-wrap: wrap;
overflow-x: hidden;
border-radius: 9px;
max-width: 300px; /* Add the max-width here */
}
/* then remove the max-width on screens larger than 992px */
@media (min-width: 992px) {
.card-wrapper {
max-width: none;
}
}
Marked as helpful
@DALO-eng
Submitted
The main problem I had was the image background-color, I do not know if I have to combine between the given colours or find a similar one :c
@byronbyron
Posted
The background was a right pain on this one! It might be overkill, but I managed to get really close with this:
.card-image {
background: url(images/image-header-mobile.jpg) no-repeat center var(--soft-violet);
background-size: cover;
background-blend-mode: multiply;
width: 327px;
height: 240px;
position: relative;
opacity: 0.75;
}
.card::before {
content: '';
display: block;
position: absolute;
top: 0;
left: 0;
width: 327px;
height: 240px;
background-color: hsl(277deg 64% 61%);
}
The image is a background-image with background-blend-mode: multiply; with a touch of opacity (0.75). Then behind that, I put a purple background pseudo element.
@braien-machado
Submitted
Is there a simpler way to change opacity of the nft image when hovering the view icon with only HTML and CSS or the Adjacent-Sibling Selector solution is the best?
@byronbyron
Posted
Hi @braien-machado
You can put a hover on the img-container to show the eye icon and cyan background as a pseudo element. Something like...
<div class="img-container">
<img src="./images/image-equilibrium.jpg" alt="equilibrium" class="card-img">
</div>
.img-container {
position relative;
}
.img-container::before {
content: '';
background: url('./image/icon-view.svg') center no-repeat hsla(178, 100%, 50%, 0.5);
display: block;
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
z-index: 100;
visibility: hidden;
opacity: 0;
transition: visibility 0.3s, opacity 0.3s;
}
.img-container:hover::before {
visibility: visible;
opacity: 1;
}
Hope that helps 👍
Marked as helpful