Skip to content
OverviewSolutions
0
Comments
68

Mike Hayden-Moore

@mickygingerLondon, UK1,005 points

Hello! 👋 I've been a professional web developer for about 12 years now, mostly full-stack. I was an instructor for 4 years teaching coding at a bootcamp in London, and now I'm the CTO and lead developer on Frontend Mentor! 🎉 I look forward to sharing my code knowledge with you!

Latest solutions

No solutions submitted yet.

Latest comments

  • Mike Hayden-Moore•1,005
    @mickygingerPosted over 2 years ago

    Hi Antony!

    Firstly, this looks great, so well done!

    If you look at the accessibility report above you should see some improvements that you should consider best practice. You can click on the Learn more link for more info and directions on how to resolve these issues.

    Probably most important is to consider semantic markup when structuring your HTML.

    I would also favour using an absolute path eg: /images/image-qr-code.png over a relative path eg: ./images/image-qr-code.png for your image links. This can be a bit tricky when developing locally if you load your index.html file directly into the browser, rather than using a webserver and running you site on localhost.

    In any case I would recommend serving your files using a webserver on localhost in your development environment. Check out this StackOverflow post for more info on running your project on localhost.

    Hope that was helpful!

    Marked as helpful
    1
  • Mike Hayden-Moore•1,005
    @mickygingerPosted over 2 years ago

    Hey Mauger,

    This looks great! I think @Yavanha has left some great feedback above, but just to reiterate the most salient point: when you name a function you will overwrite an existing function of the same name:

    function hello() {
  console.log('hello');
}

function hello() {
   console.log('goodbye');
}

hello(); // this will log "goodbye" because the second function has overwritten the first

    There's loads of different approaches to this challenge but I think the way I would do it is to assign the textContent property of the button (ie the actual text inside the button), to a variable on click. Then you can display that in the popup.

    If you think about each HTML element as an object that you can interrogate (or get properties from), then you can write a function that asks the button that was clicked what its text content is and then append that to the popup. Something like this:

    <div class="numbers">
  <button>1</button>
  <button>2</button>
  <button>3</button>
  <button>4</button>
  <button>5</button>
</div>
<section class="popup">
  <div class="result">You selected ? out of 5</div>
</section>

    const buttons = document.querySelectorAll('button'); // get ALL the buttons in one go
const popup = document.querySelector('.popup');
const result = document.querySelector('.result');

function handleClick() {
  const selected = this.textContent // get the text content of the button that was clicked
  this.classList.add('clicked'); // `this` refers to the button that was clicked
  result.textContent = `You selected ${selected} out of 5` // update the text of the popup with the selected amount
  popup.classList.add('open-popup');
}

buttons.forEach(function(button) {
  button.addEventListener('click', handleClick); // assign the click handler to each button
}

    I think this approach also means you can simplify your css a little. Here's a little more info on this in JavaScript. It's kinda nuanced and weird but very powerful! Don't expect to get it straight away, but simply put it refers to the thing that called the function. So for click events the button that was clicked, for scroll events the window that was scrolled, for keyup events, the key that was depressed...

    Oh, also you should use button and not div for the buttons. Firstly for semantic reasons but also for accessibility.

    Hopefully that helps. Let me know if you have any questions :)

    1
  • Mike Hayden-Moore•1,005
    @mickygingerPosted almost 3 years ago

    Hi Akshita, this looks great, well done.

    I think you just need to be award of the semantics of your HTML.

    You need to have one and only one h1 tag on your page. Looking at the design perhaps that should be Join our community.

    I think semantically "$29 per month" is a single element, and probably not a heading, so perhaps your should change your markup to something like:

    <p class="subscription__month">
  <span class="subscription__dollar">$29</span> per month
</p>

    Hopefully that's helpful! :D

    Marked as helpful
    0
  • Mike Hayden-Moore•1,005
    @mickygingerPosted almost 3 years ago

    Hey RyanC, this looks great!

    I'm not familiar with Svelte but checking localStorage, the countries-color-setting value never changes when you toggle dark mode.

    I think the problem might be on this line:

    if (darkModeSaved != null) darkModeSaved = darkModeSaved === 'true';

    The logic is quite complex because you're attempting to save the string values true and false in localStorage which are both truthy, and you also have to deal with null.

    I would instead use the 1 and 0, rather than 'true' and 'false'. I would also probably call the localStorage key something like isLightMode because it's a bit more idiomatic, and the default (or null value) would be dark mode on (or isLightMode: 0).

    This should make your toggle method a little easier to reason about:

    function toggleLightMode() {
  const isLightMode = +localStorage.getItem('isLightMode');
  localStorage.setItem('isLightMode', isLightMode ? 0 : 1);
}

    You'll notice the "+" in front of localStorage that converts the value returned from getItem into a number, so '1' becomes 1, '0' becomes 0 and null also becomes '0'. This is called a unary operator.

    In the setItem method I'm using a turnary operator, this is basically an inline if / else statement. If isLightMode is truthy (ie not 0), then set isLightMode to 1 otherwise set it to 0.

    Now you should be able to make your load method a bit more terse... something like:

    export async function load() {
  if (!browser) return {};

  let isLightMode = +localStorage.getItem('isLightMode');
  let prefersDarkTheme = window.matchMedia('(prefers-color-scheme: dark)').matches;

  return {
    props: {
      darkModeEnabled: !isLightMode ?? prefersDarkTheme
    }
  };
}
    Marked as helpful
    2
  • Mike Hayden-Moore•1,005
    @mickygingerPosted almost 3 years ago

    Hey Hunter this looks great!

    Since it's your first JS project, I thought I'd give you some feedback on your JavaScript.

    You've set out all your global variables at the top of the file, which is great, and initialized some sensible defaults. I think perhaps cardOne and cardTwo are not particularly descriptive variables, so I would probably recommend calling them ratingCard and successCard or similar. This helps to reason about the code.

    You've misspelled rating which is very minor, but is probably worth changing for clarity.

    Since 0 is falsey in JavaScript you can tidy up your submit button click handler a little:

    submitBtn.addEventListener("click", () => {
  if (!ratting) return error.classList.remove("hidden");

  selectedRatting.innerHTML = `You selected ${ratting} out of 5`;
  cardOne.classList.add("hidden");
  cardTwo.classList.remove("hidden");
})

    The return keyword will prevent the rest of the function logic from running so you don't need an else clause in that case.

    You have named your removeActive function, but used anonymous arrow functions elsewhere. Personally I prefer named functions since you get more specific error messaging, and you can more easily add and remove event handlers that way. Something like:

     function handleSubmit() {
  if (!ratting) return error.classList.remove("hidden");

  selectedRatting.innerHTML = `You selected ${ratting} out of 5`;
  cardOne.classList.add("hidden");
  cardTwo.classList.remove("hidden");
}

submitBtn.addEventListener("click", handleSubmit)

    Finally you don't really need to use data attributes here because the value is stored as the text content of the button albeit a string, but that's quite easy to convert to a number:

    ratting = Number(rattingBtn.textContent); // or +rattingBtn.textContent

    It's worth mentioning these are all very minor style points and should be considered suggestions and not the "correct" way to write JavaScript. What you have written is easy to read, and is not overly complex in its solution, so very well done!

    Marked as helpful
    2
  • Mike Hayden-Moore•1,005
    @mickygingerPosted over 3 years ago

    Hey Deepak, this looks great! 🎉

    I would advise that you make your mobile styles the default and modify them on larger breakpoints using min-width media queries, rather than the other way round.

    You can also add a nice box-shadow effect to match the design. box-shadow generators like this one are a nice way to experiment before adding the styles to your CSS.

    Hope that's helpful!

    Marked as helpful
    0
View more comments