Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions0Comments68

All comments

  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    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
  • Maugerโ€ข 210

    @mauger1998

    Submitted

    So this is literally the first time I have ever used javascript, I managed to get a few things working but the issue I am having is when I click on one of the ratings its always the number 1 that turns orange, I also have no idea how to get the message to tell you how many stars you have selected, if anybody can help me I would be extremely greatful I have been watching youtube videos for hours, tried debugging with the devtools and read loads of articles online and I cant find the answer.

    Update

    I have now tried giving each number a unique id but it still does not work and making sure the variable names are not the same as the ids just incase this is confusing but now when I click on any number the 5 is orange instead of the one. also when the pop up comes on it is not in the same place as the card was, someone please help

    Update The click function is working now thanks to making all the functions seperate names, the pop up still comes up in a different place to the card depending on the screen size tho and I still cant get the message to tell you how many stars you gave

    First time EVER using javascript did alot of youtubing

    #accessibility#foundation#animation

    4

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    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โ€ข 1,005

    @mickyginger

    Posted

    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
  • RyanCโ€ข 270

    @Co1eCas3

    Submitted

    There's a bug still where the theme is reverting to the initial state on navigation. If anyone can point out what is causing that, would be grateful. Currently makes absolutely no sense to me...

    Countries API w/ SvelteKit

    #svelte

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    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
  • Hunterโ€ข 80

    @huntoor

    Submitted

    My first project using javascript. Is my solution for the project good? And how can I improve it?

    All feedback is welcome!

    JS DOM manipulation

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    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โ€ข 1,005

    @mickyginger

    Posted

    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
  • David Kalanyโ€ข 50

    @AmazingCukr

    Submitted

    Hi all,

    this is my 1st attempt for this challange. I tried to make it little bit responsive but it doesnt exactly match the original image tho. Also I dont know how to change color for that picture. All criticism/help is much welcome :)

    Stats preview card component challenge hub

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey David,

    This looks great on desktop, but as you alluded to above the mobile layout is a little off.

    You've added a couple of empty divs, presumably for layout purposes? We shouldn't really be adding unnecessary markup, padding and margin (or gap in the case of grid) should be sufficient.

    You're using grid which is great, but perhaps flexbox would be a better idea here. If you aim to do the mobile layout first you can set flex-direction: column so that the image sits above the content, then change to flex-direction: row-reverse on desktop so that the image sits to the right of the content.

    Here's a really useful guide to flexbox from CSS-Tricks, hopefully you'll find it helpful ๐Ÿ˜€

    0
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Greeshma! ๐Ÿ‘‹

    First of all, this looks great, so well done! ๐ŸŽ‰

    You're always going to have difficulties when using absolute positioning for layout, so I would strongly advise against it. Absolute positioning is good when you want to position something over the top of something else, and in relation to it. If you look at the notification bell icon on Frontend Mentor, you'll see that there's a red notch that appears indicating how many unread notifications you have. That's absolutely positioned, but mostly we use flexbox or grid to position our elements on the page.

    You can position your order summary component by with flexbox like so:

    body {
  margin: 0;
}

.main-block {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  height: 100vh;
}

    Then remove position: absolute and left: 36% from .sub-block.

    Here's a great guide to flexbox by CSS-Tricks.

    Hope that helps! ๐Ÿ˜€

    Marked as helpful

    0
  • Nabilโ€ข 160

    @Nabil19911

    Submitted

    Please Rate my code. your feedbacks will help me improve in my journey.

    HTML5, SaSS

    2

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey Nabil! ๐Ÿ‘‹

    You've done a great job here, so well done! ๐ŸŽ‰

    I think there may be a simpler way to position your cards. Generally I would avoid absolute positioning if you can possibly help it.

    How about this for your tablet styles:

    .cards {
  display: flex;
  flex-direction: row;
  max-width: 700px;
  flex-wrap: wrap;
  justify-content: space-evenly;
}

.cards .card {
  margin-bottom: 10px;
}

    Then update them on desktop like so

    .cards {
  max-width: 1120px;
}

.cards .card {
  margin-bottom: 25px;
}

.cards .card:nth-child(1), .cards .card:nth-child(4) {
  transform: translateY(-50%);
}

    Using transform: translateY is nice because you don't have to set explicit sizes, so the card will always move by half its own height, regardless of the content.

    Hope that helps! ๐Ÿค“

    Marked as helpful

    2
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Yasser! ๐Ÿ‘‹

    This is cool! I have a couple of small suggestions:

    The background image is cropped on larger screens. I'm not sure the best approach here, but I think that setting background-size: cover; background-position: 50vh; might make it look reasonable on most viewport sizes.

    If you set align-items: flex-end on div.cards the two panels will line up by their bottom edges which matches the design.

    The only other thing I would recommend is that you take a look at the progress element. It's not the easiest thing to style, but it makes sense semantically here.

    I hope that's useful! ๐Ÿค“

    Marked as helpful

    1
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey Luisana!

    This looks really good, and I agree with @civisky, centering the component would be a nice touch.

    You can do that using flexbox, something like:

    body {
    margin: 0; /* remove the annoying margin added by the browser */
    height: 100vh; /* set the height to match the viewport height */
    display: flex;
    flex-direction: column; /* make sure the footer sits below the content */
    justify-content: center; /* center the contents vertically */
}

    Hope that helps!

    Marked as helpful

    0
  • Eugeneโ€ข 135

    @Eugene44-hub

    Submitted

    I also added an additional feature, dark and light mode. Please i need a feedback on the view especially. And also i noticed that the time doesn't countdown on some phones. please your feedbacks will really help me thanks.

    flexbox

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Eugene! ๐Ÿ‘‹

    Firstly this looks great, and I love that you've added your own spin to the design with the Christmas theme.

    The setInterval in your code is probably not behaving quite as you expect. The second argument is the delay between executions of the callback in milliseconds. So for example:

    setTimeout(() => {
  console.log('Hi Eugene');
}, 5000)

    Would print Hi Eugene to the console every 5000ms (or 5s) for ever (and ever and ever...).

    You've set the delay to Infinity which by some strange twist of JavaScript logic is converted into 0, which means you're executing the callback as soon as possible at the time, but realistically probably 100s or 1000s of times a second.

    Since you only need the clock to tick every second, you can change Infinity to something a little less demanding like 1000 or perhaps 500 if you want the clock to tick closer to the actual time your system clock ticks. Anything lower than 60 is redundant because that's likely to be your screen refresh rate, so you wouldn't notice it at all.

    You can stop the setInterval with clearInterval. This StackOverflow post has some good examples.

    You should also grab your clock's DOM elements outside of the callback, because otherwise you're retrieving them from the page every time the callback runs (which is an expensive operation for JavaScript). Something like this will be a lot more efficient:

    const days = document.querySelector('#days');
const hours = document.querySelector('#hours');
const minutes = document.querySelector('#minutes');
const seconds = document.querySelector('#seconds');

setInterval(e => {
  // rest of code 
}, 1000);

    OK, I hope that all makes sense. Please let me know if you have any questions!

    1
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Huรฟne Lรช! ๐Ÿ‘‹

    This looks great!

    I would be careful about using percentages for margin as you can get very unexpected results on different viewport widths.

    I like that you've used the semantic tags header, main, and footer, but I think in this instance the main content is the component. header and footer probably make more sense in terms of an entire web page, or perhaps and article.

    I would probably advise you set up your HTML like so:

    <body>
  <main>
    <div id="component">
      <img />
      <h1>Order Summary</h1>
      <p>...</p>
      <div>...</div>
      <a class="primary" href="...">Proceed to payment</a>
      <a class="secondary" href="...">Cancel order</a>
    </div>
  </main>
  <footer>...</footer>
</body>

    You can then set the background image on the main tag, and center the component using flexbox:

    main {
  display: flex;
  align-items: center;
  justify-content: center;
}

    I would then set a max-width on the component:

    #component {
  max-width: 425px;
}

    Hope that's useful! ๐Ÿค“

    Marked as helpful

    1
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Berie! ๐Ÿ‘‹

    This is really nice. I like that you've used flexbox and CSS grid, well done! ๐ŸŽ‰

    You've set the body to 100vh and then added 1rem of margin, which means your body is 100vh + 2rem (1rem top and 1rem bottom margin). Given that the component is centered in the middle of the screen with flex, I think you can afford to remove the margin from the body completely.

    I think you can make your CSS a little simpler by setting the section padding to be 35px, rather than 35px 46px 0 35px.

    You've wrapped a submit button in a link, which is invalid. Since there's no form in the component it probably makes more sense that "Sign up" is a link to a /sign-up page. I think you could apply the input[submit] styles to the a tag but you'd need to also set display: block.

    Finally you can add the shadow to the link using box-shadow. Here's a good generator: https://neumorphism.io

    Hope that's helpful ๐Ÿค“

    Marked as helpful

    1
  • Matiasโ€ข 330

    @mmc1999

    Submitted

    I have two problems that I cannot solve, please, I need recommendations:

    1. The first is that I have to click twice on the "shorten it" button for the short link to appear.
    2. The second is that I don't know how to change the background color of the network icons, I can only give them opacity. Thank you!

    responsive design with css, html y js

    2

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Matias! ๐Ÿ‘‹

    The reason you have to click the "shorted it" button twice, is because if there is no enlaces array in localStorage you skip adding the URL to the DOM:

    const mostrarDatos = () => {
    enlaces = JSON.parse(localStorage.getItem("enlaces"));

    if(enlaces === null){
        enlaces = [];
        // then do nothing...
    } else {
       // add the URL to the DOM
    }
}

    How about this:

    const mostrarDatos = () => {
    // if `enlaces` does not exist in localStorage parse "[]"
   // which will set `enlaces` to an empty array
    enlaces = JSON.parse(localStorage.getItem("enlaces") || '[]'); 

    if(enlaces.length > 0) {
        $sectionBorrar.style.display = "block";
    } 

    $insertarEnlace.innerHTML = "";

    enlaces.forEach(el => {
        $insertarEnlace.innerHTML+=`
            <div class="divAgregado">
                <p class="enlaceOriginal">${el.enlaceOriginal}</p>
                <div class="divAdentroGenerado">
                    <p class="enlaceOriginal enlacecorto">${el.enlaceCorto}</p>
                    <button class="botonParaCopiar">copy</button>
                </div>
            </div>`;
    })
}

    I'm not sure what you mean by "network icons" but you can change SVGs by opening the file and modifying the code. If you let me know which icons you want to change the background image of I'll be happy to take another look.

    Hope that helps!

    0
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Kashish! ๐Ÿ‘‹

    You've done a great job here, well done!

    Whenever you find yourself repeating chunks of code, it's a sign that you need a function, for example:

    allNotesTab.addEventListener("click", () => {
    allNotesTab.classList.add("active");
    activeNotesTab.classList.remove("active");
    completedNotesTab.classList.remove("active");

    allListSection.style.display = "block";
    activeListSection.style.display = "none";
    completedListSection.style.display = "none";
    updatingNotes()
});
activeNotesTab.addEventListener("click", () => {
    allNotesTab.classList.remove("active");
    activeNotesTab.classList.add("active");
    completedNotesTab.classList.remove("active");

    allListSection.style.display = "none";
    activeListSection.style.display = "block";
    completedListSection.style.display = "none";
    updatingNotes()
});
completedNotesTab.addEventListener("click", () => {
    allNotesTab.classList.remove("active");
    activeNotesTab.classList.remove("active");
    completedNotesTab.classList.add("active");

    allListSection.style.display = "none";
    activeListSection.style.display = "none";
    completedListSection.style.display = "block";
    updatingNotes()
});

    All the event listeners are the same but you have created three anonymous functions here. Each of these takes up space in memory, and if you want to make a change you have to do it in several places which means that it's quite easy to introduce bugs.

    This can be simplified like so:

    const handleTabClick = () => {
    allNotesTab.classList.add("active");
    activeNotesTab.classList.remove("active");
    completedNotesTab.classList.remove("active");

    allListSection.style.display = "block";
    activeListSection.style.display = "none";
    completedListSection.style.display = "none";
    updatingNotes()
});

allNotesTab.addEventListener('click', handleTabClick);
activeNotesTab.addEventListener('click', handleTabClick);
completedNotesTab.addEventListener('click', handleTabClick);

    This creates one function in memory, and changing that one function updates the behaviour of all the tabs at the same time.

    I notice you're loading in underscore, but you only seem to be using it in one place in your code. Adding external resources adds weight to your site and while it's not that important for this project, it's good to keep that in mind. The more stuff you load in the slower the site which is often undesirable.

    If you just need to check whether an object is empty there are a few ways you can do it. One fairly simple way is:

    function isEmpty(obj) {
  return JSON.stringify(obj) === '{}';
}

    Here's some info on JSON.stringify and what it's doing here.

    Finally I notice that you are storing "true" and "false" in localStorage to persist the theme setting. The problem with "true" and "false" is that they are both truthy!

    It might be better to store "1" instead of true, and "0" instead of false. You can easily convert these to numbers by using the unary operator. This will allow you to simplify your code:

    const isDarkMode = +localStorage.getItem("darkMode");

localStorage.setItem('darkMode', isDarkMode ? 0 : 1);

    If you're not sure what isDarkMode ? 0 : 1 is doing checkout the ternary operator

    Alright, I appreciate there's a lot of stuff there, hopefully you can make sense of my ramblings ๐Ÿ˜œ

    Be sure to let me know if you have any questions! ๐Ÿ‘

    Marked as helpful

    2
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Sky,

    I think I may be able to help with your JavaScript ๐Ÿค—

    Firstly, you are calling the updateValue method onchange and passing this.value, however this is not defined until the method is called so it's evaluating to undefined inside the method.

    Secondly, in updateCheck you are setting the content of #per-month to be the result of setting #dollar to be "$" + val + ".00" (which is just the string "$15"), which means that the #dollar span is being removed.

    How about something like this:

    const check = document.getElementById('check');
const dollar = document.getElementById('dollar');
const interval = document.getElementById('interval');

function updateCheck() {
  const isYearly = check.checked
  const amount = isYearly ? '$20.00' : '$15.00';
  const interval = isYearly ? '/yearly' : '/monthly';

  dollar.textContent = amount;
  interval.textContent = interval;
}

    With an added span in your HTML

    <p id="per-month">
  <span id="dollar">$16.00</span>
  <span id="interval">/month</span>
</p>

    This feels a little easier to reason about. I have stored the DOM elements in variables outside the function so that we don't need to retrieve the elements from the DOM each time we click on the checkbox (this is actually quite an expensive operation for JS).

    I'm using a ternary operator to decide which values to display based on whether the checkbox is checked, which makes the code a little more terse.

    You could (or maybe should?) also assign the event listener in the JavaScript code which would then separate the responsibilities of the HTML and JS files. Something like:

    check.addEventListener('change', updateCheck);

    I would also probably advocate changing the name of updateCheck to updateAmount, since the function changes the amount displayed to the user rather than modifying the checkbox in any way. This may seem a bit picky, but it helps to make it easier for another developer to quickly understand what your code is doing.

    Hopefully that all makes sense! ๐Ÿ˜€

    Marked as helpful

    1
  • Berta Plumaโ€ข 40

    @bertapsan

    Submitted

    Hi, please check my "solution", any feedback will be more than welcome, appreciate any testing suggestion ;-) Note: take into account I have had no access to the sketch or the Figma file.

    Social Media Dashboard toggle

    2

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Berta, this looks great! ๐Ÿ˜„

    I like your approach for setting the theme by loading in different stylesheets but I wonder if it would be more performant to set a class on the HTML when the user toggles the theme, then apply different styles based on that class... Something like:

    function handleToggle(e) {
  const htmlElem = document.querySelector('html')
  htmlElem.classList.remove('light', 'dark');
  htmlElem.classList.add(e.target.checked ? 'dark' : 'light');
}

    Then in your SCSS:

    /* base styles here */

html.dark {
  /* dark styles here */
}

html.light {
  /* light styles here */
}

    Hope that's helpful!

    Marked as helpful

    0
  • Benjo Quilarioโ€ข 1,810

    @benjoquilario

    Submitted

    Hello๐Ÿ‘‹

    This is my 20th frontendmentor challenge. At first sight I really thought it easy but I guess It's not, because this is the first time I encounter double email validation, but still with some experiment and playing around I did it.

    Feel free to drop you feedback and suggestion, I really appreciate it.

    Thanks! ๐Ÿ˜

    Mobile first - Fylo landing page (HTML-CSS/SASS)

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey Benjo, this is great! ๐ŸŽ‰

    I think you can get more of a curve on the grey panel at the bottom, by using the SVG just at the bottom of the .hero div and then setting the background color of .about to match the SVG. You'll need to remove the margin at the bottom of the .hero and use padding instead.

    You've done a great job with the email validation, but I don't think you need to get the errorMessage span at the start of your script. You could retrieve the relevant span by traversing from event.target, either using nextElementSibling or parentElement.querySelector('.hero__form--error'):

    function setFormState(input, errorMessage, className, message) {
  input.classList.add(className);
  errorMessage.classList.add(className);
  errorMessage.textContent = message;
  errorMessage.style.animation = 'errorPop 350ms ease';
  setTimeout(() => {
    input.classList.remove('error', 'success');
    errorMessage.classList.remove('error success');
    errorMessage.style.animation = 'none';
  }, 3000);
}

function checkEmail(event) {
  event.preventDefault();
  const input = event.target.email;
  const errorMessage = input.nextElementSibling;
  if (!validateEmail(input.value)) {
    setFormState(input, errorMessage, 'error', 'Invalid Email, Please check your email');
  } else {
    setFormState(input, errorMessage, 'success', 'Email Successfully Submitted');
  }
}

    I've also removed some of the duplication by creating a setFormState method, which hopefully makes the code a little less busy and easier to read.

    Finally I would recommend removing your commented code, it's not a big deal but it's the kind of thing that makes your code look a bit unprofessional, so it's probably better to remove it especially if you are applying for junior roles.

    Hope that helps! ๐Ÿ˜„

    Marked as helpful

    1
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey LenyPython, good work!

    You've set min-width: 360px on .App .content which means that your image will be larger than the viewport on smaller mobiles. I think removing that might be a good first step to getting that image looking good on mobile.

    Hopefully that's helpful!

    1
  • Terezaโ€ข 605

    @sirriah

    Submitted

    Hello,

    • I am not sure how to set the logo link to be accessible for screenreaders etc.
    • This is first time I used the srcset property - what do you think about it?

    I really like this challenge. I didn't have so much time last weeks to work on the new challenges.

    Typemaster landing page / SASS, BEM, Mobile First

    2

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hi Tereza, this looks great! ๐Ÿ˜€

    You can test accessibility by turning on your screen reader (most likely your operating system has one, for MacOS it's called VoiceOver, which you can activate in the system preferences).

    Currently you can focus on the logo link using tab, and the screen reader will say something along the lines of "visited, link, home link" which I think is probably reasonable. You don't need to set tabIndex because the link will accept focus via the tab key by default.

    An alternative option would be to use the aria-label attribute on the link, which would mean you could get rid of the span.sr-only.

    Hopefully that's helpful!

    Marked as helpful

    0
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    This is really well done Artem! ๐ŸŽ‰

    You could improve things a little by using some semantic HTML tags. For example you have <div class="header"> and <div class="article"> where you could (or should) use <header> and <article> respectively.

    Here's a good article about semantic HTML for you to have a look at: https://www.freecodecamp.org/news/semantic-html5-elements/

    Another thing I noticed is that your code is poorly formatted, so the opening and closing tags don't line up and there's lots of whitespace that should be removed. This makes it more difficult to read through and review your code.

    I would advise you use a code formatter that will automatically format your code for you, so you don't have to worry about it! Prettier is my favourite. Most code editors like VSCode and Sublime Text come with Prettier plugins.

    Hope that helps!

    Marked as helpful

    1
  • Sebin CMโ€ข 210

    @sebzz2k2

    Submitted

    The desktop site is totally messed up. Feedback of any kind is accepted. Thankyou in advance :-)

    HTML,CSS and Javascript

    1

    Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey @sebzz2k2, this is looking really nice!

    You can ensure that the background fills the entire page by adding min-height: 100vh to the body element.

    If you add position: relative to your images div, then you can position the images relative to that. So something like:

    .images {
  position: relative;
}

img.desk-top {
  position: absolute;
  left: -400px;
  top: 30px
}

    If you then add overflow: hidden to .acc_menu it will prevent the image from bleeding over the edge of the div.

    Because you want the .box image to straddle the acc_menu and the background, you'll need to place it in main. You can then repeat the process on the main to position the .box image where you want it.

    Hope that helps!

    Marked as helpful

    0
  • Mike Haydenโ€ข 1,005

    @mickyginger

    Posted

    Hey Sabrina,

    I've managed to generate a new screenshot for you. I'm not sure why it didn't work the first time, but perhaps GitHub pages hadn't completely finished deploying your site when you submitted your solution.

    ๐Ÿ‘

    0