Skip to content
OverviewSolutions
43
Comments
412
tediko
@tediko

All comments

  • tediko•6,700
    @tediko

    Hi!

    You should not use create-react-app to setup and start your React projects. It is no longer recommended by React developer team and was removed from the official documentation. CRA has problems with its performance. It is slow and bulky compared to the modern methods. It also is outdated as the dependencies themselves suffer from warnings during installation. There are few alternatives. I personally use Vite, which I recommend.

    We shouldn't manipulate the DOM directly. React is all about being declarative and so manually selecting DOM elements, manipulating them and attaching event listeners like you did in <NavBar> component is not really the React way of doing things. React utilizes a Virtual DOM to optimize updates to the actual DOM do direct manipulating can interfere with this process, leading to inconsistencies between virtual DOM and the actual DOM. This can cause unexpected behavior, visual glitches and bad performance. Additionally, React components have a lifecycle that dictates when they mount, update, and unmount. Directly manipulating the DOM can disrupt this lifecycle, making it difficult for React to manage component states and updates correctly. For instance, if you remove a DOM element directly, React may not be aware of this change, leading to errors trying to re-render that component. Instead of manipulating DOM directly and using useEffect to handle that side effect you should create local state for that component using useState hook and later on whenever your menu is toggled change that state with event handler using setter function.

    const [isMenuOpen, setIsMenuOpen] = useState(true);

function handleToggleMenu() {
    setIsMenuOpen(prevIsMenuOpen => !prevIsMenuOpen);
}

    And with that, you can attach event handler to your menu button like so:

    <button
    className="text-gray-700 focus:outline-none focus:text-blue-600"
    onClick={handleToggleMenu}
>

    And to make it work, you have to conditionally add class names for mobile-menu within your JSX.

    <div
    id="mobile-menu"
    className={`${isMenuActive ? '-translate-y-full translate-y-0' : ''}`}
 >

    Changing isMenuActive state with handleToggleMenu event handler will trigger re-render of the component which will generate new JSX with updated classes.

    In components where you are returning single DOM element you don't have to wrap returned JSX in <></>. <Fragment> lets you group elements without a wrapper node.

    Have fun!

    Marked as helpful
  • Laura Nguyen•130
    @laura-nguyen
    What are you most proud of, and what would you do differently next time?

    I utilized grid-template-areas to create the perfect grid layout for the desktop view. I also timed myself while working on this component and was able to complete it in 1.5 hours, which was faster than I had anticipated. I chose to time myself in order to improve my time management skills and practice working under pressure.

    What challenges did you encounter, and how did you overcome them?

    The cards don't have the same height as each other so initially the grid looked very jumbled. I fixed this by setting the height to 100% on the cards.

    What specific areas of your project would you like help with?

    I would love to learn another efficient way to achieve this grid layout.

    Responsive testimonial section using Sass

    #sass/scss
    1
    tediko•6,700
    @tediko

    Hello @laura-nguyen! The reason why you need to explicitly set height to 100% on the cards is because of how you set your grid container in first place. The fr unit allows us to define grid tracks as fractions of the available space which means if a grid container has 2 rows with 1fr each, they will each take up an equal amount of space, i.e., each row will be 50% of the available space. You're defining grid-template-rows: repeat(2, 1fr); so both of your rows take up equal amount of space, and since your first row card content is bigger than second row cards it will still divide them with equal amout of space leaving second row with cards jumbled as you said. Instead what you want to use is min-content keyword which is the smallest size a box can take without overflowing its content - like so: grid-template-rows: repeat(2, min-content). This way, your second row will be as small as your largest card in that row. Now you can remove height: 100% from .card container and from .card--5 where you set height: 100% and auto within some breakpoint. Last thing you need to do is remove align-items: flex-start because it is doing excatly what you want to avoid. align-items: flex-start aligns items to be flush with the start edge of their cell and instead you want to fill the whole width of the cell with align-items: stretch. But since stretch is default behaviour (align-items: normal) you don't have to explicitly set that, just remove old property.

    Since you're using display: grid for <main> on desktop you should be consistent and also use grid on mobile instead of flex. Why switch between the two if one can do excatly the same?

    Have fun!

    Marked as helpful
  • mardon1dev•60
    @mardon1dev
    What are you most proud of, and what would you do differently next time?

    This challenge takes time because of the hovering effect and responsive design.

    What challenges did you encounter, and how did you overcome them?

    Correct placing of contents

    What specific areas of your project would you like help with?

    Responsive design, I think

    For this challenge I used HTML and CSS.

    1
    tediko•6,700
    @tediko

    Hello @mardon1dev

    Congrats on finishing another project! Here is my feedback:

    • <main> is a block-level element so it takes up horizontal space by default. There is no need to set width: 100vh to it. It stretches 100% wide by default.
    • Use min-height: 100vh instead height on <main> element. By doing this your element will be at least 100% of the browser height but can be bigger if needed.
    • You shouldn't set height on .card component. Instead let your inner content decide how much space it need and how high your container will be.
    • Text should never be in span or div alone. Always use meaningful element. Change .card-learning and .card-publish to the appropriate elements.
    • Never use pixels for font-size's. Instead use rem which is relative to font-size of the root element (most browsers set it to 16px). Defining your elements font-size in pixels will not respect the user's font-size preferences and therefore your web page will not be user-friendly.
    • Usually line-height is written as a unitless value, like 1.5.

    Have fun!

    Marked as helpful
  • Daniel Wiśniewski•30
    @TheWicha
    What are you most proud of, and what would you do differently next time?

    ill use tailwind for sure next time

    What challenges did you encounter, and how did you overcome them?

    none

    What specific areas of your project would you like help with?

    none in this case

    blog-card

    1
    tediko•6,700
    @tediko

    Hi Daniel!

    You did great! Here are some tips from me:

    • By default all web browsers apply a certain amount of default styling to HTML documents eg. h1 are larger that h3, links are blue and underlined etc. These browser defaults don't go away when you add your custom stylesheet to a document. Reduce browser inconsistencies in things like default line heights, margins and font sizes of headings, and so on by using CSS Reset (e.g by Andy Bell)
    • Now that you added some CSS Reset your body element doesn't have default margin so your .card stick to screen edges on mobile screen size devices. Add padding: 1rem or so to your body.
    • Change body to take min-height: 100vh. 100vh means that the initial body height will take 100% of the viewport height, whereas the use of min-height instead of height will let the body element grow even more if necessary.
    • Your document lacks landmarks. Landmarks are a group of HTML tags and roles that define different subsections of a website and help navigate through a website. Your .card container should be <main> element.
    • Since .article-img is decorative and decorative images do not need to be announced by the screen reader, leave the alt attribute empty alt="" so it will not be announced to the user.
    • Text should never be in div or span elements alone. Always use a meaningful element. Change .label div and .user-info span to paragraph (<p>) element.
    • Your page is lacking <h1> element. h1 represent the main heading/subject for the whole app. Also, do not skip heading levels - start with <h1>, then use <h2>, and so on. Either switch your .card h2 to h1 or you can add h1 element for your app and hide it with .sr-only class since your app doesn't have title.

    Happy coding!

    Marked as helpful
  • P
    Franci Melink•400
    @francimelink
    What are you most proud of, and what would you do differently next time?

    Basically, the project was mainly aimed at the basics of HTML structure and CSS itself. Mistakes are still possible as I am basically still a beginner.

    What challenges did you encounter, and how did you overcome them?

    As I mentioned above, the project served as a basis for me. I had no particular problems with the project itself

    What specific areas of your project would you like help with?

    I definitely want to progress in HTML structuring itself, because I still often find myself editing the HTML structure itself. I think this would be a good base when I go on to more difficult projects

    Blog preview card trainig accessibility and CSS basics

    #accessibility#lighthouse
    1
    tediko•6,700
    @tediko

    Hello @francimelink!

    Good job on this challenge. Some things I'd change:

    • The <header> should not be included inside the <main> tag. When a <header> is nested in <main> , <article> , or <section> , it just identifies it as the header for that section and isn't a landmark. Adding loads of headers in sub sections or inside landmarks creates a load of unwanted noise for assistive tech users and means you then have to label every section and header.
    • You shouldn't define font-size in your body element and certainly not in pixels. Browsers set the HTML font size to 16px by default. Defining your body element font-size in pixels will not respect the user's font-size preferences and therefore your web page will not be user-friendly.
    • You shouldn't use <button> for "learning" label. Buttons are to be used for performing an in-page action, trigger some action. This is just some text with styles.
    • Author avatar should contain alt attribute since it isn't decorative image. You shouldn't hide it with aria-hidden attribute.
    • Change body to take min-height: 100vh. 100vh means that the initial body height will take 100% of the viewport height, whereas the use of min-height instead of height will let the body element grow even more if necessary.

    Happy coding!

    Marked as helpful
  • Roksana•330
    @tloxiu
    What specific areas of your project would you like help with?

    When I checked the preview of phones on my PC in web dev tools everything works fine, but when I am checking it on my phone, elements are broken, and out of their place, I will be looking into this, but if anyone have an idea, I will be happy to listen to that! :)

    Frontend Development Workflow of Tip Calculator App

    #accessibility
    2
    tediko•6,700
    @tediko

    Hi @tloxiu!

    Congrats on finishing another project! Here is my feedback:

    • Your logo image conveys no important information necessary for the user to understand the page content so it should be decorative. Keep alt attribute empty or wrap your logo with a element so it point's to home page like your alt text is saying.
    • You should add text-align: right for your .input-bill and .input-people-number inputs instead of adding odd padding like you did (padding: 0.4rem 0 0.4rem 17rem;). If user input is long enough it will cut it since there is no space (i know it won't be case in this scenario but it is just bad practice).
    • I am using Firefox and in your inputs there are arrows. Find a way to remove them to match design.
    • I believe using buttons for selecting tip is just wrong. Instead you should use input with type="radio" and the custom tip should be a radio input that when selected reveals a input type="number".
    • It is hard to use your solution with keyboard. First of all you should add some :focus-visible styles for focused elements. Then it'd be nice to "calculate" when user click enter in last input field. Now it only calculates when i click on selected tip %.
    • There is a bug when i input numbers like: bill: 66, num of people: 3 and click on 15% tip, the output is: $3.3000000000003 and it should be fixed to two points after decimal point.
    • Using the !important rule in CSS is generally considered to be bad practice because it overrides all other styles. You probably did something wrong if you have to use it.

    Happy coding!

    Marked as helpful
  • Roksana•330
    @tloxiu
    What are you most proud of, and what would you do differently next time?

    I most proud of making bigger script in js, cause i want to learn it so badd, and I guess I would do differently the years, months and day calculation, maybe with some library the next time.

    What challenges did you encounter, and how did you overcome them?
    What specific areas of your project would you like help with?

    I would like to hear an opinion about the accessibility, exactly the ARIA attributes things, and I would like the help with positioning the button on desktop design properly.

    Frontend Development Workflow of Age Calculator App

    #accessibility
    2
    tediko•6,700
    @tediko

    Hello @tloxiu!

    I can't keep up with your pace! Good job on this challenge. Some things I'd change:

    • Don’t combine landmarks like main with the role attribute. It is redundant. Semantic elements each have an implicit role. A landmark is an abstract role for a section of content that allow assistive technologies to navigate and to find content quickly.
    • Wrap your divs that contain inputs and labels with <fieldset> element, which is used to group several controls as well as labels within a form.
    • Instead of hiding your label with .sr-only class, remove your .input-title paragraph and show label instead. That's what they are for in cases like this where you have visible label in design.
    • Remove aria-labelledby in your inputs. This is what label is for. To represent a caption for an item in a user interface which in your case is input element. for attribute in label should always point to id of proper input.
    • Remove aria-labelledby from .result-section section. First of all it doesn't point to anything since you don't have Results-heading id on any of this headings. Adding labels to sections that already have headings creates a load of annoying unnecessary noise. Most screen reader users rely on headings for navigation.
    • h1 is reserved for title of the page only, you shouldn't have multiple h1's on page. I'd wrap these results with one <p> tag instead.
    • You can additionally add h1 element for your app and hide it with .sr-only since your app doesn't have title.
    • Move your styles from form to fieldset and remove max-width. Instead, add that max-width on your .wrapper element like width: 100%; max-width: 720px. Remember to hide default border on fieldset. Remember to remove min-width and max-width from .card since we set that to .wrapper container.
    • Add some class name for input and label div wrappers and style them with max-width: 160px; width: 100%; so they don't grow more than expected in design.
    • Instead of having section for divider and button, move that section within your form (outside of fieldset) and change tag to div. Replace <hr> element with div and add height: 2px; background: your-color.

    Have fun!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hello @tloxiu!

    Great to see another challenge solution from you! A few suggestions from me:

    • Move header and footer out of main. The main content area consists of content that is directly related to or expands upon the central topic of a document or an application where header/footer excludes contant that is repeated across a set of documents such as site navigation links, logos, banners etc which means we can consider them as adjacent content.
    • Your .logo is decorative so keep your alt text empty and add aria-label attribute to your anchor instead so non-sighted users know where this link is pointing to. Something like aria-label="Ping - Home" would be nice.
    • You should add label for your input and hide it with some sr-only class. The <label> element represents a caption for your input.
    • .social-medias is clearly a navigation. Nav element represents a section of a page whose purpose is to provide navigation links, either within the current document or to other documents which is true in your case. Change div tag to <nav> and next you should wrap your links inside unordered-list <ul>. Add aria-label attributes to your links to announce where those links are pointing to non-sighted users.
    • You're using broad names for color variables within your css. Try to make them more descriptive so you can use them easier within your code. Each color have its own name so instead prefix them with --c- or --clr- like --c-blue, --c-blue-pale it will be easier for you to write code since if you start typing --c it will display you all colors and then if you have different variables for maybe fonts, background etc they'll be grouped like --c-, --bg-, --font- etc.
    • Let's take a look at input and button elements. You don't want to make width of your input using padding. Instead remove all paddings values from .email-input and set it to padding: 1rem for now (you can modify it later to fit design). Next, add max-width: 350px to allow your input to take 100% width and limit it at 350px so it wont grow bigger. Next, remove padding from button for now. You want your button to take 100% height of input, and space that is left within container. To do so, just add flex: 1 property to your button so it stretches full avaible width and height. As you can see now your button lacks of height on mobile resolution. You need to match height of input so this is where you add padding: 1rem just like you add to your input.

    Have fun!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hello!

    Congrats on finishing another project! Thanks for providing your thought process aswell as asking detailed questions! Here is my feedback:

    • To me, this image isn't decorative. Anywhere an image adds any kind of value for sighted users, then alternative text should be provided so you should use img element with alt text instead. If the image is in context, and is related to your content or adds something to it it's likely not decorative. read more
    • Answering your question why image-color isn't matching design. There should be some opacity on your image to make your blend color brighter. Normally you would put background-color on .card-img and opacity: 0.75 with mix-blend-mode on img but since you went other approach with background-image as you thought your image is decorative you can't put opacity on your .card-img since it will change opacity for both container with background-color and image within. Instead use ::before pseudo-element on .card-img and put your image there so you have both container and image in different elements. Don't forget to add position: relative to .card-img.
      position: absolute;
  content: '';
  background-image: url(images/image-header-desktop.jpg);
  inset: 0;
  mix-blend-mode: multiply;
  opacity: 0.75;
    • I wouldn't sweat about making your solution pixel perfect. Of course you want to match design but don't waste your time to move elements one pixel here and there. Later when you'll have access to figma and design files taking those measurements like spacings, sizing etc will be easier so you will match "pixel perfect" design.
    • As for why you struggle with responsivnes. Look at mobile-first approach which is designing a desktop site starting with the mobile version, which is then adapted to larger screens. But since you make the other way around here's why it doesn't work for you. You should change order of flex-items when you switch to mobile screen size so your image is first, and content is second. Add order: 1 to .card-info. Next, you need to get rid of those explicitly set width/height values. You never want to do that. You want your content to take as many space as it needs and it will make you container width/height. Add max-width: 1110px; width: 100%; flex-direction: column to your .container and remove width/height. Remove width from .card-info and padding. Then add something like padding: 1rem. Remove width and set some height on your .card-img like height: 400px; width: 100%; so it take space within your container. Add padding: 1rem to your body so your container doesn't stick to device edge of the screen. Remove padding-right from .stats--item. Now you have your mobile version. From there you can make it so it match desktop design.
    • You should definitely take a read how to make your solutions responsive. read more.

    Have fun!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hi @tloxiu!

    Congrats on completing your next challenge! A little feedback from me:

    • .dice-icon div element should be replaced with button tag. Buttons trigger some action, such as drawing next advice in your case.
    • .dice-img image is purely decorative. You should remove alt text from that image since it doesn't add information to the content of a page. Instead add aria-label="generate next advice" to your button to inform users that use some form of assistive technology what kind of behaviour they can expect.
    • Instead of putting image within your button, try to use ::before pseudo-element to add this image to button element.
    • You shouldn't put two images into your HTML and remove one with display: none property. It will cause performance issues since both images need to load on each screen size. Instead use <picture> HTML element for responsive images. This element allow to display identical image content, just larger or smaller depending on the device. This helps to improve performance across different devices.
    • Don't set explicitly width on your .card container. Let your content decide how much space it need. It will make your container more responsive. Use max-width instead to say how big it can grow. Add this to your card: max-width: 33.8rem; width: 100%;
    • Now you have to add padding to your body since your container want to stretch full screen width. Add something like padding: 1rem.

    Good luck!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hello @ArdaBozan! Good job on this challenge! Here's my few suggestions:

    • Don't separate your HTML class names with "|". Separate the class names with a space, e.g. <div class="class1 class2">.
    • I'd use only one paragraph for your .price and .opacity text. Wrap opacity text into span element and style it. This will also get rid of .price-block div.
    • Your <button> element should be <a> anchor. Links take the user to a new location, such as a new web page or new section of the current page, buttons trigger some action, such as showing content on the page that was previously hidden, playing a video, or submitting a form. To me "Sign up" will take user to new page.

    Have fun!

    Marked as helpful
  • tediko•6,700
    @tediko

    Congrats on finishing your first challenge! 🎉

    I don't know Grid yet so take my advice with a grain of salt but your .card container doesn't stretch to max-width: 375px because when you aligned your grid items in body element with place-content: center your browser implicitly created tracks (both columns and rows) to align your .card to the center of the screen. Since columns were created implicitly they're set to auto which means they take only that much space that your .card container needs. What you need to do is explicitly set grid-template-columns: 1fr in your body so it stretch across whole body width and then use justify-self: center on .card element to align itself to the center of body. Also add width: 100% to your .card container so it stretch to your max-width now.

    Your document lacks landmarks, lists and have multiple heading elements that are unnecessary. Landmarks are a group of HTML tags and roles that define different subsections of a website and help navigate through a website. Your .card container should be <main> element, .buttons container could be a <ul> list and <button> elements should be <a> anchors. Links take the user to a new location, such as a new web page or new section of the current page, bButtons trigger some action, such as showing content on the page that was previously hidden, playing a video, or submitting a form. Then keep your <h1> heading as it is your component title, but change <h2> element to <p>.

    Have fun!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hi @maishamir!

    • "Making a site responsive" does not mean that every site / component needs a media query. Social links profile is the perfect challenge to create responsive component without using media queries.
    • I think you need to go back and spent more time learning HTML. Your document lacks landmarks, lists, anchors and have multiple heading elements that are unnecessary. Landmarks are a group of HTML tags and roles that define different subsections of a website and help navigate through a website. Your .card container should be <main> element, .links container could be a <ul> list and <p class="socials"> elements should be <a> anchors. Then keep your <h1> heading as it is your component title, but change <h2> element to <p>.
    • For your image provide more descriptive alt attribute for the non-sighted users.
    • Block elements (like html , body , div , p ) have width 100% automatically so width is not required for body element, remove that property from your css rules.
    • Change body to take min-height: 100vh. 100vh means that the initial body height will take 100% of the viewport height, whereas the use of min-height instead of height will let the body element grow even more if necessary.
    • You shouldn't set height on your container. Your content should tell container how tall it can grow itself.
    • You shouldn't use percentages to set dimensions of your container. % is relative to its parental container, which in your solution is body element. That mean when your .card have width: 28% and I open your website on mobile (e.g 375px wide), your .card component will have width of 100px only, which is not readable nor responsive. Instead use max-width: 390px (or rems if you read about them) and width: 100% so it will respect your parent container width, but never grow more than 390px. It will instantly make your container responsive without media-queries. The same applies for your margins/paddings. Use px or rem values.
    • For your image use explicit width and height values like 100px. Have fun!
    Marked as helpful
  • tediko•6,700
    @tediko

    Hello @davidsonaguiar,

    Congrats on finishing another project! There is a few thing that you can change:

    • Your image isn't decorative. Provide alt attribute with description for the non-sighted users.
    • Why do you select both body and html elements in your CSS? There is no need to apply style rules to both of them, so stick only with body {} selector.
    • Block elements (like html , body , div , p ) have width 100% automatically so width is not required for body element, remove that property from your css rules.
    • Since you used justify-content and align-items properties in your body to align your flex items both horizontally and vertically there is no need to add margin: auto to your main element which is now your flex item. Don't Repeat Yourself.
    • Headings are signposts that guide readers through component. Therefore, they should indicate what a section or a paragraph is about. Keep your <h1> heading which is title of your component but change <h2> to <p> element.
    • You forgot to wrap your list items content within anchor tags <a> so they can point to external website.

    Good luck!

    Marked as helpful
  • tediko•6,700
    @tediko

    Hello @ArdaBozan!

    Nice work on this challenge! There is some things you can fix:

    • Use alt attribute for your img element. It provides alternate text for an image if the image cannot be displayed or is displayed by assistive technologies such as screen readers. It is required.
    • You shouldn't define font-size in your body element and certainly not in pixels. Browsers set the HTML font size to 16px by default. Defining your body element font-size in pixels will not respect the user's font-size preferences and therefore your web page will not be user-friendly.
    • Use a elements instead of buttons. Buttons are for performing an in-page action and anchors/links are for navigating to a new page.
  • Anyona Zaddock Ogada•190
    @zacc-anyona
    What are you most proud of, and what would you do differently next time?

    What I am proud of in this project

    • I am proud that I managed to build a fully responsive web page using flex-box. I love flexing my muscles in this area.
    • Proud of the fact I managed to overcome some of the challenges I encountered and went to ahead to complete the challenge.

    What I would do different next time

    • I reduce the number of media queries I have in my CSS by employing mobile-first workflow.
    What challenges did you encounter, and how did you overcome them?

    Challenges I encountered in this challenge

    • Making the site responsive by the use of media queries.
    What specific areas of your project would you like help with?

    I need help in the following areas;

    • Responsiveness
    • How to come up with descriptive class names.

    Blog Preview Card

    #accessibility#lighthouse
    2
    tediko•6,700
    @tediko

    Hello @zacc-anyona!

    Good job on this challenge! There is a few things that you can improve in your code and learn a thing or two alongside!

    • You shouldn't define font-size in your body element in pixels. You don't have to do it at all. By default, browsers set the HTML font size to 16px. A lot of people change their base text size in browser to make it more accessible for them. Defining your body element font-size in pixels will not respect the user's font-size preferences. Remove font-size from body to use default browser size.
    • Learn about landmarks because you're using them wrong. Landmarks are semantic elements that can be used to define different parts of a web page to make your website accessible to users using assistive technologies. Landmakrs help screen reader users navigate your website. The <main> element represents the dominant content of a document. That's why you need to wrap your .content div with <main> element. If you want your .content element can be an <article> but it isn't necessary. Change your main element within .content to div. <header> is element that represents a container for introductory content or a set of navigational links. In your example either of these conditions are met so change <header> to normal <div> element.
    • Read about figure element since you misuse it within your header element. The <figure> element represents self-contained content, and <figure-caption> provides the accessible description to that element. Replace it with div element. As for using this figure for the avatar, I think it is a more reasonable choice, but still don't know if I'd use it for that.
    • Instead of setting width: 30% on your .content component, you could use max-width property to set the maximum width of an element.
  • tediko•6,700
    @tediko

    Hello, Chamu! 👋

    Congrats on finishing another challenge! 🎉 Your solution looks very good and also responds well. Here's some feedback:

    1. You left a lot of console.log methods in your code. Get rid of this as it makes the code harder to read.
    2. A common convention which is widely followed is that component should follow PascalCase naming convention. The name of the folder/filename will always reflect what is present in that file that's why it is good to also use PascalCase for them, not only function within that file. e.g. (Header/Header.js)
    3. You should avoid using document.querySelector with React. There is only few cases (sometimes there is still a need for manipulating elements directly) where it can be useful but in most cases there is better way to approach that. Instead you can use useRef() hook. Refs are more reliable for getting the specific DOM node you care about. document.querySelector can give unexpected results if there are multiple elements that match the query and if you're writing a component that will show up multiple times in many places this problem will occur, that's why refs become more of a clear-cut winner.
    4. Take a look at themeControl function in header.js component. You used document.querySelector for three elements which is completely unnecessary. As you know React provides a way to inject javascript right into our HTML using something called JSX interpolation. So instead of taking element from DOM with querySelector and then inject some data with .innerHTML or .src methods you should simply create a expression inside your HTML elemetns like so:
    /* If theme is true return "Dark" if not return "Light". */
<span className="mode__state">{theme ? "Dark" : "Light"}</span>

/* If theme is true return Moon element if not return Sun element. */
<img className="mode__img" src={theme ? Moon : Sun} alt="" />
    • To completely get rid of querySelector from this function we can use document.body property. It represents the <body> node of the current document. Along with this, I used conditional (ternary) operator to either add or remove theme-dark class. Now themeControl() look like this:
     const themeControl = () => {
    theme
      ? document.body.classList.add("theme-dark")
      : document.body.classList.remove("theme-dark");
  };
    • Now you can remove themeControl() call from handleClick since you want this to perform everytime your theme change. For this purpose in useEffect with themeControl function, add theme to dependency array. The dependency array in useEffect lets you specify the conditions to trigger it. So in our case we'll watch for every theme change and trigger useEffect only when that value change, like so:
    useEffect(() => {
    themeControl();
}, [theme]);
    1. I made a small refactor of your localStorage save option in header.js. In functional programming it is good to break up your code into small reusable functions. For this, we will create two functions, one to save data to localStorage and one for retrieving that data.
    • Let's start with postThemeToLocalStorage function where we will set our newTheme value to globalTheme key. Note we don't need to include the window property. You can access built-in properties of the window object without having to type window. prefix. The window object is the top-level object of the object hierarchy. As such, whenever an object method or property is referenced in a script without the object name and dot prefix it is assumed by JavaScript to be a member of the window object. So our function will look like this:
    const postThemeToLocalStorage = (newTheme) => {
	localStorage.setItem("globalTheme", newTheme);
};
    • Then let's create getThemeFromLocalStorage function where we want to return our globalTheme key value from localStorage. You don't need to use JSON.parse() method since our value from localStorage is a string, not an object. As the localStorage value cannot be saved as boolean we want to simply convert it. For this, we make a simple comparison, to return either true or false but as boolean values, like so:
    const getThemeFromLocalStorage = () => {
    return localStorage.getItem("globalTheme") === "true";
};
    • Now, we have to clean up the code a little since we have a few unnecessary things here. Remove localStorage and savedTheme variables. Then, in handleClick function, instead of manually save our data to localStorage, use postThemeToLocalStorage(!theme). Then, add useEffect hook, that will only trigger on render (by providing empty dependency array) and save our theme from localStorage to theme state, like so:
    useEffect(() => {
    setTheme(getThemeFromLocalStorage());
}, []);
    • Finally, we have to assign a starting value for our theme state. Since you don't use anything like prefers-color-scheme to detect if the user has requested a light or dark color theme, we assign theme to be false (light theme) - const [theme, setTheme] = useState(false). Previously, when your localStorage was empty you got null value from storage. null is a falsy value so it still works, but we want to avoid such cases.

    I'll send you my copy of header.js on slack just if I forgot to include here something. Good luck with that, have fun coding! 💪

    Marked as helpful
  • tediko•6,700
    @tediko

    Hello, Gutka! 👋

    Congrats on finishing another challenge! 🎉 Your solution looks good. Here's my few tips:

    • Less breakpoints doesn't necessary means better. Now, your solution isn't fully responsible. It does look good on mobile and on larger desktops but everything inbetween does look like mobile and it isn't good. Here is my old solution to this challenge just to illustrate you how it can behave across screen sizes.
    • You repeat your HTML code with star images which is unnecessary. Find the way to use only one star image. You can do it easily by using background-repeat approach in your css. To do this you'll have to create empty div and remove all img's.

    Good luck with that, have fun coding! 💪

    Marked as helpful