@Maritxx
Submitted
Hi! this is my solution for the tip calculator project. I struggled with the custom button (updating the value for that) and the updating of values when the input changes. Is there a smart way of doing this?
Looking to hire developers?
@ErayBarslan
@Maritxx
Submitted
Hi! this is my solution for the tip calculator project. I struggled with the custom button (updating the value for that) and the updating of values when the input changes. Is there a smart way of doing this?
@ErayBarslan
Posted
Hey, great work on the challenge! You can use input
eventListener to achieve that. For custom input it would be:
document.getElementById("custom__button").addEventListener('input', (e) => {
calculateTipAmount(e, e.target.value / 100)
})
You can apply the same to other inputs. Happy coding :)
@catherineisonline
Submitted
Hello, Frontend Mentor community. This is my solution to the Crowdfunding product challenge.
Happy to hear feedback and advice!
@ErayBarslan
Posted
Hey Catherine, excellent work on the challenge! There is just a little gotcha about background-size
. Header has fixed height so contain
won't let the image grow as the screen gets wider. Everything else looks perfect, happy coding :)
Finally finished this challenge. There were difficulties with the positioning of the main image. Feedback is welcome
@ErayBarslan
Posted
Hey Darya, design is almost pixel perfect and responsive so nothing much to add, excellent work! Some suggestions:
aria-label
or title
attributes. Or you can simply use it inside <main>
.<p>
for texts. Also you can get rid of nested divs like in here:<header>
<div class="wrapper">
<div class="header">
/* content */
</div>
</div>
</header>
All 3 containers are parent of the content and you can achieve the same by just using <header>
without the need of divs. In regards of accessibility nested divs won't mean much because they're ignored, just there for styling purposes. Although not using unnecessary elements will keep your code more maintainable and easier to work on for others. Aside these everything looks great and happy coding :)
@andykallian
Submitted
Hello!
It's my first time trying to implement a dark mode theme, and it was more difficult than i've thought.
First of all, creating an empty div and then populate it with classes and events through JS code required a lot of attention as they were "a div inside another div inside another div...and so on"
after, I couldn't turn 'checkbox' input into rounded instead a square box, so I created a div (through JS) and then put a style on it in css. Visually speaking, it worked! Functionally speaking? well...I can't confirm this since I still haven't added the functionality to confirm the check by the user.
Anyway, my logic in JS seems to be working, but I don't believe it's following good patterns.
if anyone can give any suggestions related to the whole project, it will be welcome and I will be grateful!
I hope you enjoy it! Regards!
P.S: I still intend implement media queries, but for now it's ok
@ErayBarslan
Posted
Hey Anderson. Great work on your solution! To use checkbox and style as you wish you can use appearance: none
. This is what I've used on my solution:
.theme-switch {
appearance: none;
width: 26px;
height: 26px;
background-image: url('images/icon-sun.svg');
background-repeat: no-repeat;
cursor: pointer;
}
.theme-switch:checked {
background-image: url('images/icon-moon.svg');
background-position: center;
}
Also you can define just one class per theme on body. Example:
.dark {
--bg-color: hsl(235, 21%, 11%);
--bg-content: hsl(235, 24%, 19%);
}
.light {
--bg-color: rgb(235, 235, 235);
--bg-content: white;
}
Then add & remove with checkbox. You can use the variables on any element, switching class from body will be enough. Would make it much more easier to store in localStorage as well. Happy coding :)
@trandainien
Submitted
All comments are welcome
@ErayBarslan
Posted
Hey there, congrats on your solution! Design looks good, responsive and you have clean usage of CSS. Nothing much to add but some suggestions:
.attribution {
...
position: absolute;
bottom: 0;
}
body {
...
justify-content: center;
}
/* In this case you can place your container to center */
background-color: var(--Very-light-gray);
Although arguably white looks better so you might as well leave it as it is..container
and .attribution
you can use semantic elements to make the page accessible: <main class="container">
& <footer class="attribution">
alt
empty. Screen readers skips empty alt. Since images on this challenge are for decorative usage, nothing wrong regarding accessibility. But your page might get lower SEO. Instead you can use like : <img src="./images/icon-sedans.svg" alt="sedan" aria-hidden="true"/>
target="_blank"
, adding rel="noopener noreferrer"
will make it secure to attacks. Aside these nothing I'd add, happy coding :)Marked as helpful
@Aleroms
Submitted
How's my scss structure?
@ErayBarslan
Posted
Hey there, excellent work with the challenge! Design looks good and accessible. Your scss usage looks alright and for such project you won't need more of it's features. By the time projects starts getting bigger you can start structuring your scss files like _variables, _mixins and import to main. But would be redundant for this one. Some suggestions.
width: 375px
value on container. If you use max-width
you'll see it's responsive for smaller screens aswell. By default width: auto
which is taking available space to fit the content inside it. In this case you can increase the value to match the desktop design by keeping it same for 375 screen width: max-width: 450px;
margin: 0
to body it'll fix that. For your next projects I'd suggest resetting all padding and margin at the start of your project by using * { ... }
selector. So you'd have full control over your elements. Aside these nothing I'd add, happy coding :)Marked as helpful
@stepheigbe
Submitted
How do i makes sure the coded by stays at the bottom?
@ErayBarslan
Posted
Hey there, great work on your solution! To place attribution on bottom and not effect the rest of your layout you can use:
.attribution {
font-size: 11px;
position: absolute;
bottom: 0;
}
Absolute positions the element relative to it's parent without effecting any other element. Since parent of attribution is the main, you'd want to move it out of main and make it's parent body. To keep it semantic, you can use <footer>
on attribution instead of div. Hope this answers your questions, happy coding :)
@nekokanbaru
Submitted
I tried to input the close button svg using svgsprite to change the color on hover, but when I did it the element was there but you couldn't see the icon, so I changed it back to a regular img.
Another problem I had is that I could load tasks from local JSON but not save them, I looked it up on google but everywhere I looked people were using node, so if someone knows any other way I could do that I would be grateful if someone could tell me.
Otherwise it was a very fun project.
@ErayBarslan
Posted
Hello Sylvanas! Great work on the challenge. It's responsive and everything works fine.
visibility:hidden
by default and turn it to visible on hover to achieve that effect.localStorage
. Whenever user creates a todo you can use localStorage.setItem() method to save the data and call with localStorage.getItem() method. You'd need some checking but it's pretty straightforward. If you wish to see on usage you can check my solution of the challenge which I used localStorage: Todo App.
Hope these answers your questions, happy coding :)Marked as helpful
@AmanGupta1703
Submitted
.container::before { content: ""; background: url("/images/pattern-background-desktop.svg") center center/contain; position: absolute; width: 100%; height: 26rem; top: 0; left: 0; z-index: -1; } I am able to see the output of the above code in the visual studio live server, but I can't see it in the GitHub live page.
I would like to hear your feedback! 😊
@ErayBarslan
Posted
Hey there, nice work with your solution! Some suggestions:
.summary-box
without effecting the design. Removing .container
will require little refactoring but you can remove anything related to that.body {
...
background: url("/images/pattern-background-desktop.svg");
background-repeat: repeat-x;
}
.summary-item {
...
width: 90%;
max-width: 414px;
}
As you can see, just adding max-width
is enough to achieve the same thing. You can also lower the need of media-query by adjusting %
values for the mobile version, then give it a max-width
. This approach is also preventing content overflowing between 375-550px due to width: 48%
.
.summary-item
as container, you can use <main>
on it instead div to be semantically right. Aside these good work again and happy coding :)Marked as helpful
@satzzzzz07
Submitted
Hey All, Open to feedback and suggestions!!
@ErayBarslan
Posted
Hey there, excellent work on your solution! Design looks good and everything works as supposed to. My suggestions:
bill.addEventListener("input", () => { billGen(); renderBill() })
customTip.addEventListener("input", () => { customTipGenerator(); renderBill() })
people.addEventListener("input", () => { peopleCountGenerator(); renderBill() })
/* Since there are two functions for each listener, we put them inside another function. */
input[type="number"]::-webkit-outer-spin-button,
input[type="number"]::-webkit-inner-spin-button {
-webkit-appearance: none;
}
input[type="number"] {
-moz-appearance: textfield;
}
<h1>
for the heading of page. If there are several heading then It'd be applicable to use other headings..attribution
you can use <footer>
instead <div>. Aside these excellent work again and happy coding :)Marked as helpful
@Luis-Olivero
Submitted
How do I get rid of the space between the profile picture and the profile name/age?
How do I get rid of the space between the stats and the text under the stats?
Thanks for all the feed back!
@ErayBarslan
Posted
Hey Luis. Great work on your solution! It's responsive and accessible. I've looked into your code and the issue comes from top
attribute on img. You can fix it by changing it to margin-top
:
.card__img--picture img {
position: relative;
margin-top: -3rem;
border: 6px solid #fff;
}
When top attribute used, base position of the element stays same, just visually effects it's positioning. Using margin simply fixes that. For your second issue, it's due to the default margin of h2 and p. You can fix by using: h2, p {margin: 0;} To not deal with default margin, padding at the beginning of CSS in addition to box-sizing I'd suggest adding:
*,
*::before,
*::after {
box-sizing: border-box;
margin: 0;
padding: 0;
}
Aside these perfect solution and happy coding :)
Marked as helpful
@amaar09
Submitted
Updated...
@ErayBarslan
Posted
Hey there, excellent work on this one! Regarding your question, another option would be adding a return button. You can use setTimeout here but it displays only for 1,5 seconds which isn't enough for the user to read the page. It's better to use a higher value. Additional suggestions:
submit.addEventListener("click", () => {
if (a) {
/* your entire function */
}
})
max-width: 385px;
to make your container responsive. By default it's width: auto
which is taking the available space without overflowing. Giving fixed width overrides this. Aside these nothing much I'd add and happy coding :)Marked as helpful
@Jumanjigobez
Submitted
Thanks for viewing this and liking it.
Eager waiting for your feedback.
@ErayBarslan
Posted
Hey there, great work on this one! Design looks good and is responsive. My suggestions will be regarding semantic html:
aria-label="top navigation"
<a href="#"><li>...</li></a>
target="_blank"
attribute on links, adding rel="noopener noreferrer"
will make it secure. Aside these everything looks great. Hope you find these helpful, happy coding :)Marked as helpful
@MrFougasse
Submitted
My first challenge. Here it is.
@ErayBarslan
Posted
Hey there, congrats on your first solution! Design looks really close to the provided. My suggestions:
max-width
to make the page responsive. by default width: auto
which is responsive. As it is, on small screens content overflows. .wrapQR {
background-color: hsl(0, 0%, 100%);
max-width: 300px;
padding: 0 10px 30px 10px;
border: solid #fff;
border-radius: 15px;
margin: auto;
}
.QR {
width: 100%;
border-radius: 15px;
margin: 10px auto;
}
You can keep the rest of styling as it is. With these changes, base design is the same but when screen gets smaller there is no overflowing issue that is due to max-width. Also since your page is deployed now, you can use generate new screenshot option to see how close you are on your design. Hope these helps, happy coding :)
Marked as helpful
@EDDuardOo-Code
Submitted
I would like to ask how to improved with the management of spaces, using paddings or margins?
also the use of media querys and until which point these are useful ?
also, what are the best practices to work with css, flexbox and media querys?
@ErayBarslan
Posted
Hey there, congrats on your first solution! Some suggestions:
max-width
without the need of media-query.body {
min-height: 100vh;
display: flex;
justify-content: center;
align-items: center;
}
.child{
...
width: 95%;
max-width: 300px;
}
.attribution {
...
position: absolute;
bottom: 0;
}
/* position absolute on attribution perfectly centers the container */
Test it out and you'll see the difference. Also as far as html goes, you can use semantic landmarks instead using div on everything. By removing the container your html inside body can look like:
<main class="child">
<img src="./images/image-qr-code.png" alt="image-qr-code">
<br>
<div class="text">
<h1 class="mob"> Improve your front-end <br> skills by building projects </h1>
<br>
<p>Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p>
</div>
<br>
</main>
<footer class="attribution">
Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank" rel="noopener noreferrer">Frontend Mentor</a>.
/* when you link to other pages, adding rel attribute adds security */
Coded by <a href="#">Your Name Here</a>.
</footer>
I haven't touch <br> but for spacing it's better to use margin through css instead br. Hopefully these helps, happy coding :)
Marked as helpful
@munyite001
Submitted
As always, feedback is highly appreciated :)
@ErayBarslan
Posted
Hey there, excellent work with your solution! Design looks good, it's responsive and js sidebar functionality is clean. Some suggestions:
.show-sidebar {display: none;}
to your media query. .container {
max-width: 100rem;
margin: 5rem auto;
}
You can do the same to the header if you like. Aside these nothing I'd add. Excellent work again and happy coding :)
Marked as helpful
@wellintontews
Submitted
Project made with HTML and CSS, I'm looking for ideas to develop myself more and more in this magnificent world that is the front-end, all kinds of help and suggestions will be welcome
@ErayBarslan
Posted
Hey there, nice work on your solution! Some suggestions:
min-width
so you won't ever deal with overflowing issues. Also desktop layouts are usually more complex. By designing desktop first then switch mobile, we're taking that complexity away. So with mobile first approach, we'd be writing less code.height
to your containers and always let it's children, padding, margin define it's height. Won't matter much for a static card but once you start working with dynamic content, you'd end up with lots of overflowing.max-width
to keep your containers responsive. Because by default width: auto
which is responsive and giving fix value overrides it. Hope these suggestions helps you, happy coding :)@Hayamaker
Submitted
First time building a project in Frontend Mentor!! I learned a lot about how margins and paddings worked, how shadow boxes were implemented, how websites were deployed and many more. Quite excited to build more projects from here!
@ErayBarslan
Posted
Hey there, amazing work as your first challenge! Instead of margin you can center you card by the help of flex:
html {
height: 100%;
}
body {
min-height: 100%;
display: flex;
justify-content: center;
align-items: center;
}
Also I see you're trying to deploy your site using github-pages. Simply change your html file name to index.html
because pages looks for an index to run the site. Try it out then your site should be deployed with no issues. Hope these helps and happy coding :)
Marked as helpful
@salmaaboudou
Submitted
I've just completed this challenge. Any feedback and suggestions on how I can improve are very welcome 🤗
@ErayBarslan
Posted
Hey there, excellent work with your solution! Design looks good, it's responsive and form works as supposed to. There are just few things I'd add to your solution:
max-width
on container elements so that design won't break.<section>
, <article>
. If there is a fitting heading It'd be better use one for accessibility. In this case you can use <h2> for Try it for free text instead of <p>.Marked as helpful
I didn't know that i could use any css lib i wanted.
@ErayBarslan
Posted
Hey there, github pages looks for index.html on root directory. In your case all your files are inside qr-code-component-main and github pages detects no index.html. If you move all the files inside it and simply remove the file, it should deploy with no issues.
@casserole27
Submitted
I'm happy with my solution, but if anyone has any suggestions, I'm continually looking for ways to make my CSS more efficient and seamless between mobile and desktop. Thank you!
@ErayBarslan
Posted
Hey there, nice work with this one! I see you've tried to center your content but you need an addition for it to work:
html {
height: 100%;
}
body {
...
min-height: 100%;
}
.body-wrapper
to body
element. Right now background doesn't cover wide screens. Use the wrapper just to align your content.max-width
on containers instead of a fixed width
, your page becomes more responsive.min-width
value as content overflows on certain screens. These are my suggestions to improve your solution, happy coding :)Marked as helpful
@staceysav
Submitted
Feedback will be highly appreciated! While doing the project I had some difficulties; if you have answers to any of the questions below, please let me know :)
@ErayBarslan
Posted
Hey there, you can add background to body as:
body {
...
background-image: url(./images/pattern-background-desktop.svg);
background-repeat: repeat-x;
/* we can use repeat x for this specific image so that it supports wider screens*/
}
Try out box-shadow: 0 25px 20px hsl(225, 67%, 88%);
which is what I've used on my solution.
-Instead of <h4> you should be using <h1> for Order summary, each page requires a level one heading. You can style with css as you like. Also their level should change 1 by 1 so you can change <h6> to <h1>.
alt
should have meaningful explanation. Instead of alt="..." you can use alt="dancing with music illustration"
These are my suggestions to improve the solution. Happy coding :)Marked as helpful
@Cheemma1
Submitted
All feedbacks are welcomed
@ErayBarslan
Posted
Hey there, great work on your solution congrats! Some suggestions:
mix-blend-mode
without the need of ::after
and you'd get much closer to the design. If mix-blend-mode wouldn't exist that's a nice solution though! By removing after you can use:.right img {
width: 100%;
display: block;
opacity: .7;
mix-blend-mode: multiply;
}
.right {
background-color:hsl(277, 64%, 61%) ;
}
min-width
to design for bigger screens. This way you wouldn't deal with overflow and in general convenient approach is mobile first design. Also It'd be better to use max-width
on containers instead width
. By default width:auto
and is responsive, overriding it takes the responsiveness away. Aside these great work and happy coding :)@UmesiQueen
Submitted
Hey Guys, I'm really having fun completing these challenges . I found this one interesting trying to figure out how to overlay a background color on the image and Yes! it was the fastest I've completed so far 🤣
Your Feedback will be appreciated.
@ErayBarslan
Posted
Hey there, excellent work on this one and good to hear you're having fun :) Some suggestions:
.background_img
you can use mix-blend-mode: multiply;
so you'd match with design.width
on main you can use max-width
so that your page would be responsive for all sizes with the same amount of code. By default width:auto
which fills the available space. By overriding it we take the responsiveness away.<main class="container">
& <footer class="attribution">
. Your designs stays the same but page becomes more accesible. Happy coding :)Marked as helpful