Skip to content
OverviewSolutions
14
Comments
10
turtlecrab
@turtlecrab

All comments

  • turtlecrab•550
    @turtlecrabPosted over 2 years ago

    Hey, nice submission!

    There are some problems in the game logic:

    1. There are no draws. It's probably your design decision and you are aware of this, but it's not Rock Paper Scissors when your opponent knows your hand and doesn't repeat it.
    2. The opponent's choice is biased towards the Rock. This is because of how you generate the choice:
    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
    2
  • Soojeong•220
    @kongguksuSubmitted over 2 years ago

    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?

    Responsive Chat App CSS Illustration

    2
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    1
  • turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    0
  • Elias Fung•140
    @iManchaiSubmitted over 2 years ago

    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!

    Responsive Landing Page build with TailwindCSS

    1
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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:

    • The 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 here
    • Bottom divs 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)
    • Usually we don't need to add the word "image" to alt texts of images. Here is a great article about alt-texts: https://axesslab.com/alt-texts/, it explains the topic very well.
    • And in this case I think these bottom images are purely decorative, so I'd remove alt-texts from them (<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.
    • Also a little bug: if we open the menu in mobile view and then resize the window to desktop view, then the menu stays and there is no way to hide it apart from going back to mobile screen size. This situation can happen in real life if a user goes from portrait to landscape orientation on their mobile, or if they just simply resize the window on desktop. I think adding md:hidden to <div id="mobile-menu" ... should fix this.

    Hope this helps!

    Marked as helpful
    1
  • Dmitriy Smirnov•40
    @Teor99Submitted over 2 years ago

    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:

    • VSCode
    • Live Server (vscode ext)
    • Live Sass Compiler (vscode ext)
    • Chrome browser
    • PerfectPixel (chrome ext) Feedback welcome.

    NFT preview card

    2
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    3
  • JunoField•310
    @JunoFieldSubmitted over 2 years ago

    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!

    Interactive rating component using HTML, SASS, JS and Parcel

    2
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    1
  • Bashar Khdr•930
    @BasharKhdr1992Submitted over 2 years ago

    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.

    Responsive Time tracking dashboard

    1
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    0
  • turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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.

    0
  • Allyson S.•190
    @allyson-s-codeSubmitted over 2 years ago

    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

    Tip Calculator App using HTML, CSS, Flexbox, and JavaScript

    1
    turtlecrab•550
    @turtlecrabPosted over 2 years ago

    Hey great job, it looks really close to the design!

    Addressing your question - there are several inaccuracies in your code:

    • your query selector parameter string looks like you are up to grab labels for radio inputs(correct query for this case would be 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 loop
    • checked 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
    1
  • turtlecrab•550
    @turtlecrabPosted over 2 years ago

    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
    1