@KhaledSobhy10
Submitted
it is fine but need some of refactor in future :D
Looking to hire developers?
@turtlecrab
@KhaledSobhy10
Submitted
it is fine but need some of refactor in future :D
@turtlecrab
Posted
Hey, nice submission!
There are some problems in the game logic:
const guess = shapes[Math.round(Math.random() * 2)]
You round a number between 0 and 2, so:
0 < N < 0.5
= 0
0.5 < N < 1.5
= 1
1.5 < N < 2
= 2
As you can see the range that rounds to 1
is two times bigger than others. So if you choose the Paper every move, the opponent will choose the Rock in 66%
and the Scissors in 33%
. And vice versa, when you choose the Scissors you lose twice as frequent as you win.
The proper random number is:
Math.floor(Math.random() * 3)
Also there are accessibility warnings that you should check out, accessibility is important.
Hope this helps.
Marked as helpful
@kongguksu
Submitted
In the process of adding animation, I noticed that a scrollbar appears on the right side of the animated elements when the screen size is smaller(non-desktop versions) and I'm trying to search how to get rid of it, but I'm having a bit of difficulty figuring out how to do that. Is there a way to fix this?
@turtlecrab
Posted
Great job on almost perfect pixel layout!
As it was said before by @JoseLuisF, the problem with flashing scrollbars is because of overflow-y: scroll
on main
at @media(max-width: 57em)
.
Removing it does solve the problem, I just tested it both on real mobile(android) and in devtools preview. Before the fix I could see the scrollbars in both of them.
It does not remove scrollbars of the whole page - those are handled by html { overflow-y: scroll; }
It also fixes the weird looking clipping of the shadow of .app-container
Hope that helps.
Marked as helpful
@Carlosgnx
Submitted
why my reset button cant be disabled?
@turtlecrab
Posted
Hey, nice solution! Addressing your question - your reset button is in fact becomes disabled, but you don't have any styles for disabled state. Use :disabled
pseudo class to style disabled elements. Something like this:
.splitter__reset-btn:disabled {
opacity: 0.5;
cursor: default;
}
Hope this helps.
Marked as helpful
@iManchai
Submitted
Hello FEM Comunity!
This was my first time using TailwindCSS. It was a fun project to do but there were moments when I got stucked. For example working with the SVG files. Parts that I founded hard to make were the mobile menu, and to make the responsiveness of the website.
Any sugestions on how to improve my code or have better practices are welcome!
@turtlecrab
Posted
Hey, great job on this one! It works great on all screen sizes(maybe except for very very wide) and is really close to the design!
Some feedback:
main
tag should include all the following section
tags. It's semantic role is to show screen readers where the main content of the page is. More about it herediv
s that hold images belong to the footer
in my opinion, not the main
. After these two fixes the accessibility warnings should be gone as all the content would be inside landmarks (header
, main
and footer
in this case)alt
texts of images. Here is a great article about alt-texts: https://axesslab.com/alt-texts/, it explains the topic very well.<img ... alt="">
). But if you think that these(or any other) images convey some information that people who don't see them should know about, then feel free to add image descriptions to alt
.md:hidden
to <div id="mobile-menu" ...
should fix this.Hope this helps!
Marked as helpful
@Teor99
Submitted
First try with scss. There were difficulties with the Eye image icon, I'm not sure what I did right, but it seems to work. Used tools:
@turtlecrab
Posted
Hey, kudos for making it pixel perfect(I think it lacks font-weight: 600
for heading and price though), but the layout only works for 2 exact screen widths that you hardcoded in css for body
, for any in-between sizes the design is pretty much broken.
In your case the fix is very easy: just remove width
, height
and min-width
from the body and set min-height
to min-height: 100vh
. This is a standard approach to center both vertically and horizontally with flex:
body {
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
}
In general, we never want to hardcode body sizes like you did, we want our designs to be responsive(google responsive/adaptive design) so our site/app is looking good on every screen size. This is our first priority, and we must test our apps on all screen sizes(we can do it in Chrome DevTools or just simply by resizing our browser window). Making it 100% pixel-perfect is a great goal, but it's more or less among our last priorities.
Hope that helps.
Marked as helpful
@JunoField
Submitted
As a beginner with little experience, I'm certain that my code is far from good and I'm looking for constructive criticism, so please be kind but honest.
Particularly of note is the radio buttons - because of the layout and lack of an actual "check mark", making them visually show selections using CSS alone was beyond my ability. I resorted to using JS for this instead - probably a bloated and "hacked-together" approach, but the end result is functional.
Also, to be honest using SASS and Parcel was probably a mistake for this project specifically - it was not necessary and overcomplicated things, especially with regards to publishing the site via Github Pages.
Thanks in advance to anyone who decides to leave feedback - it's much appreciated!
@turtlecrab
Posted
Hey, it's very easy to make radio labels react to checked
state without any js, just put inputs and labels side by side, input then label, and use +
css selector like that: input:checked + label {...}
. That's how I did it in this challenge following BEM class naming convention:
.card__radio:checked + .card__radio-label {
background: $primary;
color: $white;
}
If you put inputs inside labels I think there is no easy way to achieve this, as far as I know there is no way to get a parent css selector from the child, and we need to bind our label selector to :checked
pseudo-class of an input(maybe I'm wrong and there is a way, but there are no reasons to make it harder). The only thing you'll have to add is for
attribute for the label like this: <label for="input-id"...
so a label knows which input to check on click.
About your javascript code which handles class toggles - it's not needed if we rewrite our layout as shown above, but as it's written I have a feedback. There are 5 almost identical functions that handle label clicks. If you haven't read about DRY programming principle yet (which stands for don't repeat yourself), then google it, it's the first principle we should be learning about. Here we can safely instead of 5 functions write just one:
function handleRadChange(number){
EnableButtonResetColours();
document.getElementById("circle-" + number).classList.add("select-circle-clicked");
}
And html:
<label id="circle-1" onclick="handleRadChange(1)" ... >
<label id="circle-2" onclick="handleRadChange(2)" ... >
...
The code is much simpler and if you decide to add some other functionality you'll have to add it to one place and not 5.
So looking for duplicate code is good and we should do it and try to remove duplication when it's making sense. But if you are a beginner don't go crazy about it, start with just noticing duplicate code and do nothing about it. Then maybe when it's really similar like in the example above start trying to refactor it if you are feeling like it. If you are not, don't bother and leave it duplicated, it's totally fine at this stage. And sometimes it's just better to leave duplicate code(see here and here)
Also you should add node_modules
directory into your .gitignore
file, as well as parcel related directories: .parcel-cache
and dist
. Just write them 1 directory per line. These are not supposed to be a part of git repository and .gitignore
is where we list such files/dirs.
If there are issues with deploying to Github Pages, try Vercel - I use it and it's just couple of clicks to add a repository and build it. I think with Vercel you don't even need to write a build script, it detects parcel automatically. Another choice is Netlify.
Marked as helpful
@BasharKhdr1992
Submitted
Your feedback is highly appreciated. What's wrong with the screenshot utility. It absolutely does not reflect the actual solution I have submitted. The dimensions are incorrect, the font is not the one I have used. Any idea how does it work. I would appreciated that.
@turtlecrab
Posted
Hey, for me the solution looks exactly like in the screenshot. I think the problem is that you didn't import the font(via css or html) and you probably have installed that font on your computer so it's working for you.
And that gap in the middle is caused by min-height
's of .dashboard
, .main-card
and .personal-details
, it looks nice on smaller screens, but breaks the layout on big ones.
Hope that helps.
Marked as helpful
@EVPina
Submitted
Is better to use max-width to min-width in Media-Query?If not, in what case is recommendable to use it?
@turtlecrab
Posted
Using min-width
is mobile first approach, it means that you first style your page for the small screen sizes, then add media queries with min-width
for tablets, desktops, tvs(it could be 1 or more breakpoints).
On the contrary, max-width
is desktop first approach - you first style for the desktop screen size, then add media queries for smaller screens.
I think mobile first is widely considered to be a better practice(google mobile first to read the arguments), but some people say that desktop first is easier and in some cases is more appropriate.
My advice would be to try them both, do a solution here with one of the approaches and the next solution with another, then compare you feelings :)
I personally did one solution here with desktop first, my motivation was to match the final design comparison screenshot as close as possible, and because it's a desktop screenshot I started with designing for that. But mobile first feels smoother to me, it's easier for me to first focus on basic stuff and smaller screen and then build up on that.
@allyson-s-code
Submitted
Hi!
I'd love any feedback on my HTML, CSS, or JavaScript. I really struggled with the styling of the :checked
state for the % buttons and after a lot of research and help from slack channel I switched from buttons
to radio inputs
. I feel pretty good about it now, except I can't seem to figure out how to remove the :checked
state on the tip inputs once the user clicks on the custom tip percent input area. I tried a few versions of this:
customTip.addEventListener("click", function () {
document
.querySelectorAll(".percent-input .percent-input.label")
.classList.remove("checked");
});
Any suggestions would be great! Thanks so much!!! Allyson
@turtlecrab
Posted
Hey great job, it looks really close to the design!
Addressing your question - there are several inaccuracies in your code:
querySelectorAll('.percent-input + label')
), but you actually don't need labels, you need to uncheck the inputs themself! So query request here should be simply querySelectorAll('.percent-input')
querySelectorAll
returns a list of nodes, so you can't just use methods like .classList.remove
on it, you need to iterate over the list and do all the stuff in that loopchecked
is not a class, it's a property of an input, so to remove it we just set it to false
So the working solution I've come up with is:
customTip.addEventListener("click", function () {
const radioInputs = document.querySelectorAll(".percent-input")
radioInputs.forEach(input => input.checked = false)
});
I hope that helped!
Marked as helpful
@BernardusPH
Submitted
Any advice would be great. Sorry about all the setProperties I am still a beginner. I want to ask why does the computer compute 10 when I type 012 or 18 when I type 022?
@turtlecrab
Posted
hey, it does so because numbers starting with 0
are octal numeric literals in javascript(octal is like hexadecimal numbers but on base 8). You can disable them by putting 'use strict'
in the beginning of you js file, and it will throw syntax error instead of parsing them. I think it's better than returning incorrect(by human standards) computations. I hope it did help!
Marked as helpful