Skip to content
OverviewSolutions
26
Comments
58
Yazdun
@Yazdun

All comments

  • Yazdun•1,310
    @YazdunPosted about 1 year ago

    This is looking great, well done!

    I have a suggestion for you, instead of hardcoding accordion data, you can store all the accordion data inside an array and map through the array inside your react component.

    This way you won't need to repeat the same code with different title and description, it also will be so much easier to update the data or change the JSX since the pattern is repeated only once.

    I've already opened a PR which resolves this issue, make sure to check it out.

    Other than that, it's looking fantastic, keep up the incredible work!

    Marked as helpful
    1
  • Shonuff•280
    @TheShonuffSubmitted over 2 years ago

    Used this challenge as an opportunity to learn Svelte. Very happy with the framework and will continue to use it in the future. If you have any thoughts or critiques I would be happy to hear them. I always enjoy learning from my mistakes. Thank you for looking at my project.

    Planet Facts

    2
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hi Joe, great job on the challenge. This looks very clean and it's also responsive on all sort of devices. I specially love the animated hamburger icon 😁

    I don't know Svelte so I can't talk about your code, but there is a really serious issue in this project which you should be aware of.

    Imagine this is a production level project, and our user is doing a research on the Jupiter. She wants to bookmark Jupiters internal structure so she can come back to it later and use it as reference, right now there is no way to point exactly to Jupiters internal structure and she must first land on the website, then click on the Jupiter, then click on the internal structure ! We will definitely lose our user right there !

    Now imagine another scenario, Someone is reading Uranus surface geology and he finds it very interesting so he decides to share it with his friend, Now whatever URL he sends to his friend, they will land on Mercury overview 😁

    This can also completely destroy the SEO since there is no way for search engines to show exact results to the users.

    The solution to this issue is, creating separate pages for each planet and then creating sub pages for each category, for example : planets.com/earth/structure . Or if you don't want create pages, you must at least implement queries so users and search engines can point to a exact planet and sub category, for example : planets.com/solar?planet=earth&category=geology .

    Overall, Great job and keep coding 👏

    Marked as helpful
    2
  • Vanza Setia•27,775
    @vanzasetiaSubmitted over 2 years ago

    Hello Everyone! 👋

    I finally finish my first API challenge! 🥳

    This is a hard challenge for me, but I learned a lot of new things. 😀

    Hopefully, I can get some feedback as well since I am still new at asynchronous programming. 😁

    Now for the questions:

    • There's an article where it tells me that I should use input with type="text" instead of type="search". But, is this a good thing to follow?
    • Should I put the role="search" on the form element or on the div element (inside the form element)? I don't know the correct answer because MDN recommends putting the role="search" on the form element. But, Ahmad Shadeed recommends to put the role="search" on the form element to prevent overriding the default form semantic.
    • I follow the GitHub alternative text of the user's avatar which is like alt="@username". But, do you think it's good enough or is there a better alternative text for the avatar?
    • If you see the site, there are some statistics such as the total of public repositories, how many followers the user has, etc. My question is, should it be read as "8 Repos" or "Repos 8"? Currently, screenreaders will read the text as "Repos 8". But, if I should change it, then I would probably use flexbox to reverse the text visually. So, screenreaders would read the text as "8 Repos".
    • Should I have a hidden heading above each list? For example, <h3>User statistics</h3>. I got the inspiration from the @grace-snow's solution for the "Profile card component" challenge).
    • There are disabled links (or not available links) in this challenge. So, let's say the janedoe's GitHub account has no link in his/her profile. So, what do you think about the way I handle the disabled links? Currently, I make them as normal text (<span>Not Available</span>).
    • Also, you might notice that janedoe has no name. So, I visually hide the h2 if the user has no name, <h2 class="sr-only">Not Available</h2>. What are your thoughts?
    • Is there a better way to manage the colors for the dark mode and light mode? I used CSS variables in this project to make it easier because I could just switch the colors with prefers-color-scheme. You can see the CSS code to see how I handle the color for this project.
    • If you can help me suggest a better name for any function in my JavaScript, I would really appreciate it. For example, setThemeSwitcherStateForDarkMode() is a very long name. 😆

    I am aware of the HTML issues that have been generated. I added the role="list" to each ul element to make sure the ul element still has the semantic meaning when I set list-style: none;.

    If you find any bugs or notice I miss something, please let me know. Also, if you spot any bad practices in my code, feel free to tell me. I will try my best to make improvements to make this solution better. 🙂

    Also, if you have finished this challenge and would like me to give feedback on it, please include a link to your solution. I would be glad to help you! 😀

    Thanks!

    GitHub User Search App | HTML CSS Sass JavaScript (Async Await)

    7
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hi Vanza, Great job on the challenge, I specially love the fact that you added a query in the url for the searched user, that's a really nice touch.

    Here is quick issue that Matt Seidel noticed on my solution and then I could fix it. Try searching mseidel819 in your app and you will see a weird overflow which doesn't look nice, you may wanna take care of that.

    3
  • Andro87•1,460
    @Andro87Submitted over 2 years ago

    This is my solution for this multipages website. Any feedback on how to improve it will be highly appreciated. ;)

    Planets fact site

    1
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hi Andro, Great job on this challenge, This is actually one of my most favorite challenges on the frontend mentor. There are three main issues with this solution :

    • Your page's layout will break on wide screens.
    • Your project is not structured properly.
    • You took a wrong approach ( I explain why in depth )

    Now I'm going explain each issue.

    1. Page's Layout : I started with this one since it's the most easiest to fix. You have written a scss class called main_container inside ./styles/Home.module.scss and used it in various places.

    .main_container {
    width: 100%;
}

    Now, You've applied this class to the elements which are block level by default and taking 100% width is their default behavior, There is no need to apply this class to them.

    But what happens if the user is browsing our website in an ultrawide monitor ? Do we want our container to fill the whole screen ? The answer depends on the project itself, but for this project the answer is clearly no, because we don't have much content on the screen so it's better to control the container's width, so you can easily update main_container :

    .main_container {
    width: 100%;
    max-width: 80rem;
    margin: auto;
}

    With the above's approach, our container fill 100% of the available space but when it hits width of 80rem, it will stop growing and we will have a better looking layout on a widescreen.

    Also you need to use background svg which was provided with the starter files.

    2. Project's structure : This mainly comes back to components directory but I'd like to mention some more issues that I would've personally change :

    • You've used SCSS for styling but you haven't used any powers of SCSS, so why not use plain CSS instead ? If you are using SCSS it's better to at least embrace some of it's powers, if not, then it's easier to remove the SCSS complexitiy from our project and use simple plain CSS instead and sharpen our fundamentals. CSS itself is super powerful !

    • It's better to create a specific folder for each component so our codebase becomes more readable to other developers. You approach is working and it's fine, But in the eye of another developer who wants to contribute to your codebase, it's extremely cluttered. So it's better to put each component inside it's own specific folder.

    • It's better to create an index.ts file inside ./components and export all components from there, so in case we want to use multiple components somewhere else, we can import them all at once from ./components/index.ts.

    • Since we are using Nextjs, Vercel highly recommends us to make use of Absolute Imports and Module path aliases. It's really easy to configure and all you need to do is adding "baseUrl": "." to the compilerOptions inside tsconfig.json.

    3. You chose a bad approach

    This is the most important one and this can really hurt your project, Now you've used a fancy carousel and it has awesome transitions, but you've created a huge issue with choosing fancy stuff over performance ! Here is why :

    • What if our users wants to share surface geology section with their freinds ? No matter what url they send to their friends they will land on planet's overview when they click on it.

    • What if someone search for venus internal structure on the internet and they don't care about venus's overview or surface geology? Shall we expect them to land on overview and then look for internal structure button and manually click them?

    • What if our user is making a research on planet Mars and she wants to bookmark Mars's surface geology so she can return to it later for multiple times? Each time she must land on overview and then choose her desired tab which becomes annoying overtime and we will definitly lose our user right there!

    The solution to this is, creating a sub route for each specific section, There will be a main page for the planet which shows the overview, and other tabs should be a separate sub page on it's own, so search engines and users can make better use of our website.

    If you want to stick with the carousel approach, You must at least implement a functionality so we can point to the specific carousel tab with the page url ( You can use a query for this ).

    Overall you did a great job, I hope this helps you to improve your coding skill. I also opened a PR to your repo which fixes some of these issues that I mentioned.

    Marked as helpful
    3
  • Dilhan Boca•220
    @dboca93Submitted over 2 years ago

    Hello,

    Feedback welcome for my first frontend mentor task. I'm new to web-dev and I'm keen to learn more. I imagine there are easier ways to complete this task using CSS Grid (which I'm currently learning) -- is this correct?

    Also, could someone comment on whether the sylistic changes I made on smaller screen sizes (via media queries) are correct ? Is the read-me file ok ? All other feedback is more than welcome. Thanks !

    QR Code Component: centered vertically and horizontally using position

    2
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hey man good job on the challenge. This project doesn't need any grid so it's fine the way you've done it ( although I can argue that it doesn't need any flexbox either 😅 ).

    The other thing is I don't see any need for media queries in this challenge. instead of using static width for the main__container , you can use something like :

    .main__container {
  width: 100%;
  max-width: 270px;
}

    With the above's approach, You won't need any media queries, main__container will occupy 100% of the screen and when it reaches 270px width, It will stop growing. You can give some paddings to the parent element to prevent main__container from hitting the screen's edges on mobile devices.

    Overall you did great, Keep it up.

    Marked as helpful
    0
  • Shonuff•280
    @TheShonuffSubmitted over 2 years ago

    This is was an interesting challenge and had a lot of little things I didn't notice when I first accepted this challenge. This was a fantastic learning experience and looking forward to another intermediate level challenge.

    Tic Tac Toe

    1
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hello Joe, Congrats on finishing this challenge ! While the project itself looks awesome, there are some issues in regard to your react's codes which I like to mention :

    • First of all, remove the files which you are not using in your project from the source code, for example you have not written any tests for your project, so you can remove the App.test.js and setupTests.js so people won't get confused when they are exploring your repo.

    • You must create separate directories for different purposes, Now all the resources are living in src directory, This is working, there is no issue with that, But in the eye of the another developer who wants to use your project this looks extremly cluttered. My recommendation is creating /components directory and put each component inside in it's own separated folder, for example /components/game/Game.js. No matter how small our project is, we must follow the best practices anyway.

    • For exporting and importing files, The best practice is creating an index.js file inside each directory and export them once from it's directory, and then importing them all at once from there. To make this more clear I give you an example : You have imported assets in each file by pointing at them like

    import Xicon from "./assets/icon-x.svg";
import Oicon from "./assets/icon-o.svg";
import logo from "./assets/logo.svg";

    Now, what if at the middle of the project we decide to change the icon-x.svg name to icon-y.svg ? You guessed it, all hell will break loose ! We must go and find each folder which uses Xicon and manually change it's name to Yicon and also fix the import path. The better way is isolate the import inside it's directory so each time we want to change something's name, we only change it at one place. So here is my recommended approach :

    // ./assets/index.js
export { default as Xicon } from "./icon-x.svg";
export { default as Oicon } from "./icon-o.svg";
export { default as logo } from "./logo.svg";
export { default as IconRestart } from "./icon-restart.svg";
export { default as OiconOutline } from "./icon-o-outline.svg";
export { default as XiconOutline } from "./icon-x-outline.svg";

    Then inside the component :

    // ./components/someComponent/SomeComponent.js
import { OiconOutline, Oicon, Xicon, XiconOutline } from "../../assets";

    • Now let's talk about structuring your JSX. First of all you have to keep something in mind when working with react, I kinda made it a rule for myself : If your component is exceeding 100 lines of code, something is wrong ! . No one tells you that, React won't throw any error, and at that moment everything works fine, but when you come back to it after a week or so, or when you need to add a feature to your app after some time, It becomes a nightmare ! ( trust me on that I learnt it the hard way ). Each component should ONLY DO ONE THING at a time, For example I'm looking at your Board component, This is the sacred temple of your project and no one has the guts to touch this file 😅 it's 253 lines of code and there are all sort of functions, loops, 11 useStates, many if and else statements and ternary operators, and then you have passed all these stuff as props to different components so if we touch anything the whole app breaks. Here is my solution for this issue, You need to isolate the game's logic inside a context and then provide it to the components which needs them. Each component should interact with the context on it's own term and should not depend on other components in order to work, this way you can split Board.js into many small sub components so you can easily make changes to them and maintain your app.

    • For styling, I recommend you to learn CSS modules alongside with classnames npm package, Using global css may cause conflicts with css classnames and cause bugs in your app, with css modules you can make sure that css classnames are isolated inside component's scope.

    • I saw you have used react-device-detect library, This library has very special usecases and for your project you could've easily created a custom useWindowWidth hook ( You can learn more about this on google, it's really easy to create ) and show components based on user's device width instead of using a library.

    ✅ That's all I had to say, I also opened a PR to your github repo which fixes some of these issues. I hope this helps you to improve your react coding skills. Overall great job and keep it up

    Marked as helpful
    2
  • Fluffy Kas•7,715
    @FluffyKasSubmitted over 2 years ago

    Hey guys,

    I'd like to apologise to anyone who's only interested in the rating card component. Just click through the pizza creation, on the finish page you can see it as a popup, if you choose to give a feedback.

    Because that's what this component turned into. A pizza app. Somehow. I wanted to practice React so I kept adding new things to it and this is what I ended up with. Design wise it's not the fanciest (I'm really not a designer, sorry) but that wasn't really the point here anyway.

    I added a couple of features: a pizza creation function, signup/login function with Firebase (password reset as well), a light-dark theme, a contact form and a pizza-loader animation (thanks to my sister for the pizza images she made for this and for the home page background). And finally there's the rating card component as a modal, which was the original challenge.

    I learned a lot in the process: I got a tiny bit better at Framer Motion, I wrote a bunch of React code, used context and Firebase for the first time. I even managed to reuse some things from my other projects (like the mobile menu) - it felt great that I didn't have to do it again from scratch.

    I appreciate if you take the time to look at it. I take any sort of feedback regarding accessibility, React best practices, css (a bit messy at the moment), animations, how to make things more reusable... anything.

    If there's anyone who's a pro at Framer Motion and can give me a hint on how to do exit animations, I'd love it.

    And lastly I hope it isn't inappropriate to upload this app here. It really started out as the rating card component, just got carried away.

    Anyway, happy Easer/have a good weekend everyone!^^

    Interactive Rating Card - turned into a modal in a pizza app

    5
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Great job on this challenge, everything is smooth and awesome 👏

    In regards to your code, There are some issues that I like to mention.

    • Components which are used as react router element, should be separated from other components. This is not crucial but it helps us to maintain our codebase easier. Usually we create a folder called /pages or /views and put our page components there.

    • For importing and exporting components, it's better to first reexport them from an index.js file inside components folder, and then when we want to import them somewhere else, we can import them all at once. To make it more clear look at the following example

    ⛔ BAD PRACTICE

    //APP.JS

import Header from "./components/header/Header";
import Home from "./components/home/Home";
import Base from "./components/base/Base";

    ✅ GOOD PRACTICE

    // components/index.js

import Header from "./header/Header";
import Home from "./home/Home";
import Base from "./base/Base";

export { Header, Home, Base };

    //APP.JS

import { Header, Home, Base } from "./components";

    This way you'll have much more cleaner syntax and also it is easier to maintain 👆

    • When multiple components are using same values, we MUST use context to avoid prop drilling. For example themes are something that are used globally in our app so we shouldn't pass it as a prop, we should isolate it inside a context and each time a components needs it, we can provide theme to it, Here is a theme context that I wrote for your app, by using useTheme() you can use theme and setTheme value across your whole application :
    import { useContext, createContext } from "react";
import useLocalStorage from "use-local-storage";

const ThemeContext = createContext();

export function ThemeProvider({ children }) {
  const defaultDark = window.matchMedia("(perfers-color-scheme: dark)").matches;
  const [theme, setTheme] = useLocalStorage(
    "theme",
    defaultDark ? "dark" : "light"
  );

  return (
    <ThemeContext.Provider value={{ theme, setTheme }}>
      {children}
    </ThemeContext.Provider>
  );
}

export function useTheme() {
  return useContext(ThemeContext);
}
    • We have same prop drilling issue for your pizzas too ! Right now you're drilling newPizza and setNewPizza into four different components, and this is red flag ! Here is a context which handles our pizzas so we can have access to newPizza and setNewPizza across our whole application :
    import { useState, useContext, createContext } from "react";

const PizzaContext = createContext();

export function PizzaProvider({ children }) {
  const [newPizza, setNewPizza] = useState({ base: "", toppings: [] });

  return (
    <PizzaContext.Provider value={{ newPizza, setNewPizza }}>
      {children}
    </PizzaContext.Provider>
  );
}

export function usePizza() {
  return useContext(PizzaContext);
}
    • As a developer, we must follow DRY principle to keep our code maintainable, repetition is a red flag whenever and where ever we see duplicated code, we must figure out a way to fix it. For example inside your RatingCard component, You've repeated the same input for 5 times with different values, this is a bad practice. Instead you can create an array of inputs ( ideally react components ) and loop through them using map() method, Here is my refactored version of your RatingCard :
    const nums = [1, 2, 3, 4, 5];

<div className="input-container">
  {nums.map((num, index) => {
    return (
      <div className="focus-box" key={index}>
        <input
          type="radio"
          name="rating"
          id={num}
          value={num}
          onClick={selectRating}
        />
        <label htmlFor={num}>{num}</label>
      </div>
    );
  })}
</div>;

    • It's better to keep react components under 100 lines, I'd say 60 or 70 lines is ideal, but as soon as your component exceed 100 lines, it's time to break it up to separate components. Although It's fine to sometimes have long jsx.

    • Right now there is way too much repetition inside your ContactPage, Each input should be separate component. I recommend you to learn React Hook Form, This library has made my life 10 times easier.

    • Use SCSS modules instead of global SCSS

    • You can use React icons for your icons, it's easier and more reliable than using external assets IMO.

    ✅ Also I opened a pull request to your repo which fixes most of the issues I mentioned, Also you can find this comment there, inside ISSUES.md.I Hope this helps.

    Marked as helpful
    10
  • Vanza Setia•27,775
    @vanzasetiaSubmitted over 2 years ago

    Hello Everyone! 👋

    Finally, I completed another PREMIUM challenge! 🎉

    It was a challenge where I needed to play around with the background images. They were tricky! I also improved my old Regular Expression from the Base Apparel challenge. I had written everything that I learned in the README file. So feel free to check it out! (and give feedback on it 😄)

    Now for the questions:

    • I added alternative text for the Spotify, Apple Podcast, Google Podcasts, and Pocket Casts logos. However, the paragraph above the form element has mentioned all the platforms. I'm not sure that they are decorative or informative images. So, what do you think about it?
    • If you try using a screen reader on my website, can you understand the page content?
    • Can you navigate this website comfortably using your keyboard?

    Any questions on the technique that I'm using are welcome! 😁

    Also, if you have finished this challenge and would like me to give feedback on it, please include a link to your solution. I would be glad to help you! 😀

    Thanks!

    Pod Request Access Landing Page | HTML CSS Sass JS (Regex)

    2
    Yazdun•1,310
    @YazdunPosted over 2 years ago

    Hello Vanza, this is so awesome man well done, your commitment is so inspiring keep it up 👏 I played around with your solution for a little bit, and as perfect as it is, I have some suggestions :

    • for error handling, I personally prefer to handle errors before submission, I declare a variable called let hasSubmitted = false; and as long as it is false, it means user has not submit the form yet, then after first submission, if there is any error, I show the error and also hasSubmitted = true; . this let me to show errors dynamically after failed submission as users type their email by using something along the following code 👇
      heroInput.addEventListener("input", e => {
    hasSubmitted && // handle errors here 
  });

    and also I disable the button as long as there is any error after first failed submission, so users won't need to click submit to find out if there is any error or not and errors will be handled as they are typing. I know this is a personal preference but I think it had worth mentioning.

    • also there is another thing : using nested if and else statement, again that is not really an issue but I'm gonna tell you my personal approach and maybe you find it useful. in your js you've used 👇
     if (isEmailInvalid) {
      event.preventDefault();
      if (heroInput.value) {
        showAlertMessage(message);
        setTimeout(hideAlertMessage, 5000);
      } else {
        showAlertMessage(message);
        setTimeout(hideAlertMessage, 5000);
      }
    }

    but I think using switch instead of nesting if and else is more readable, maybe you find following code more readable too 👇

      switch (isEmailInvalid) {
      case false:
        console.log("it's all good");
        break;
      case heroInput.value:
        showAlertMessage(errorMessage);
        setTimeout(hideAlertMessage, 5000);
        break;

      default:
        showAlertMessage(errorMessage);
        setTimeout(hideAlertMessage, 5000);
        break;
    }

    that's all I had to say and I love to know what you think about em. and here I have a question :

    • how much time do you spend on creating these challenges ? your README file is so nice and I haven't seen such a clean solution on frontend mentor, and do you still do these challenges on a mobile device ? forgive me for my curiosity though
    Marked as helpful
    2
  • Kristina225•260
    @Kristina225Submitted almost 3 years ago

    Hi, everyone. Any feedback you can give me on my solution is appreciated. I was mostly struggling with the hover state on the image. I added an empty <div> in my HTML for the cyan transparent background and changed its 'display' property with 'mouseenter' and 'mouseleave' with JavaScript. But maybe there are better ways and JavaScript is not necessary. Thank you all.

    NFT card component using HTML, CSS and JavaScript

    4
    Yazdun•1,310
    @YazdunPosted almost 3 years ago

    Hello Kristina, well done on this challenge 👏 I see you've written extremely clean code, thumbs up 👍

    • about solution itself, as you said yourself, you don't need JS for hover stats, all you need is :
    .card__image--container:hover .card__image--backdrop,
.card__image--container:hover .card__image--icon {
  display: block;
}

    in your css and you'll achieve same functionality without any javascript.

    • here there is another issue I found, you've used width: 35rem; and height: 59.6rem; on your card, so when device's width gets smaller your layout will break, it is not recommended to use static size for your elements and you must let the content specify the size, so what you can do is :
    .card {
  width: 100%;
  max-width: 35rem;
  // rest of the code ...
}

.card__image {
  display: block;
  position: relative;
  /* let the content specify width */
  max-width: 100%;
  border-radius: 0.8rem;
}

    and some padding on the body, will make sure that your layout won't break on small screens.

    • use PRETTIER extension on your code editor to format your code.

    • side issue about github 😄 : don't jam all your solutions on one branch, create separate branch for each solution, and as you are already using netlify for deployment, you can easily deploy each branch automatically using netlify, this way if you mess up one of your project, your whole repo won't go bananas and you can easily delete the corrupted branch.

    I also opened a pull request to your repo which will fix the solution issues I mentioned, keep coding

    Marked as helpful
    2
  • Tomas Scerbak•720
    @TomasScerbakSubmitted almost 3 years ago

    I have no idea how to combine HTML, CSS and JS to show different layouts when click on share button :D. Could only finish mobile version.....somehow.

    Any suggestion for best practice and how to do it would be appreciated.

    Thank you

    article-preview-component-master

    1
    Yazdun•1,310
    @YazdunPosted almost 3 years ago

    Hello dear Potato 🥔

    allright there is so many ways to get this done, I mention two main approaches I personally take for these scenarios.

    • for this challenge, I used media queries. I mean it's a same component, as long as it is within small screens it sticks to the bottom, but as soon as it hits wider screens, it completely transforms into different styles, but it is still same css class ! here is my solution on this challenge in case you wanna check it out.

    • there is another approach you can take, you can create two different elements for mobile version and desktop version, and add display:none to desktop version, then using media queries, when user reaches wider screen you can add display:block to the desktop version and display:none to the mobile version. ( I guess it got a little bit confusing but I did my best 😁 ). then using javascript you can toggle both these classes but always one of them is hidden according to the device's screen width.

    I hope you find this comment helpful

    Marked as helpful
    0
  • Yazdun•1,310
    @YazdunPosted almost 3 years ago

    Hey Raul great job on this, to get rid of accessibility issue, you must add a h1 heading to your main tag which tells what this page is about, it can be hidden, you can use a simple sr-only css class if you want to make it hidden.

    right now, as your website hits around 337px width, QR card sticks to the edge of the screen which is not the best user experience IMO, so make sure to use some paddings on QR code's parent element.

    in my opinion your box shadow is too thick, but hey, that's a personal opinion 😁

    well done and keep coding !

    Marked as helpful
    0
  • lmac-1•10
    @lmac-1Submitted about 3 years ago

    This is my first challenge on the platform. I chose it because it didn't look like I needed to do too much to make it responsive, and wanted to start with something simple. Overall I really enjoyed it, it was a big challenge for me but I'm happy with the final results.

    Looking for feedback on the following areas:

    1. Use of semantic HTML - I'm not confident I've chosen the correct elements for the design
    2. General CSS good practices - is my CSS well written? Am I missing things?
    3. Background position - I was quite stuck on this property and I'm not completely confident with this part of my solution. Is there a better approach?

    Profile card component built using HTML and CSS

    2
    Yazdun•1,310
    @YazdunPosted about 3 years ago

    Hello Lucy and well done on the challenge, Here are my suggestions :

    • Use Prettier extension on your IDE to format your code.

    • use code below instead of specifying default value for html element separately

    *,
*::before,
*::after {
  margin: 0;
  padding: 0;
  box-sizing: border-box;
}
    • Html elements are block level by default ( thats one of the reason for approaching mobile first ), so you don't need 👇
    display: flex;
    justify-content: center;
    align-items: center;
    flex-direction: column;

    on your main tag, you get the column by default

    • give min-height:100vh instead of height:100% to your body.

    • right now, background circles are not responsive and layout breaks quite easily on smaller screens, so here is what I did on this challenge for background circle to make sure of their responsiveness. firstly add 👇

    .c1 {
  right: 50%;
  bottom: 43%;
}

.c2 {
  top: 50%;
  left: 50%;
}

.c1,
.c2 {
  position: fixed;
  transition: 0.5s cubic-bezier(0.075, 0.82, 0.165, 0.6);
  z-index: -1;
}

    to your css and then add images to the markup 👇

      <img
        src="./assets/images/bg-pattern-top.svg"
        alt=""
        role="presentation"
        class="c1"
      />
      <img
        src="./assets/images/bg-pattern-bottom.svg"
        role="presentation"
        alt=""
        class="c2"
      />

    and then remove background properties from body. this way, background circles won't go bananas on smaller screens. keep in mind that's my approach and there are definitely better approaches you may come up with.

    • use width:100% and then max-width: ... px instead of giving a static width to your card, so card's layout won't break on small devices, right now your card leaves side scroll on small screens.

    • I don't recommend using static heights for elements unless it's really crucial.

    • for stats, I think you'll be better off with something like

     <ul class="card-stats">
        <li><span>80k</span> <span>Followers</span></li>
        <li><span>803k</span> <span>Likes</span></li>
        <li><span>1.4k</span> <span>Photos</span></li>
 </ul>

    instead of using so many divs.

    • give some paddings to body so content won't occupy whole screen on mobile devices.

    ✅ Also I opened a pull request to your repository which will fix these issues

    I hope this was helpful

    Marked as helpful
    2