Skip to content
OverviewSolutions
3
Comments
11
Pavel B.
@Jagholin

All comments

  • Pavel B.•270
    @JagholinPosted over 1 year ago

    My browser says that it can't load stylesheet from google fonts. It appears that the URL in the <link rel='stylesheet'> is mistyped, which means the font isnt loading. Just FYI.

    0
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Great job! I only looked in DevTools, but havent found much to complain about.

    What can be improved:

    • It's just my personal preference, but in many cases you dont need display: flex; flex-direction: column, the default flow layout does many things just fine. It also does margin collapsing, which can be either good or bad(depends whether you are used to it or not).

    • width: 100% does nothing for display: block elements, and might even be harmful sometimes

    Marked as helpful
    1
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Styling is more or less fine, some things are misaligned. Phone number input field doesnt accept spaces, even though the placeholder text suggests that it should.

    "Next step" button for some reason has different styles on different pages. Sometimes it has cursor: pointer, sometimes doesnt.

    Next step is not a <button>, "go back" is neither a link nor a button. The second page has 0 interactive elements, according to accessibility tool in DevTools

    Dont use heading tags <h1>, <h2> etc. for styling, as they have special meaning for accessibility tools. Each page should have exactly one <h1> element. Skipping heading levels is unadvisable.

    Marked as helpful
    1
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Great job!

    Some suggestions for the future:

    • inlining CSS styles into HTML can be sometimes useful(from performance standpoint). However I suggest to forget that it's even possible and add links to .css files instead. In the future when you do larger projects, separating stuff into different files will help you with organization, and bundlers like Vite will take care of compiling your code into optimised, minified representations ready to deploy to the Web.
    • position: relative doesnt do anything here.
    • The task was to display the panel in the center of viewport, which is done only 50%. There are several ways of vertically and horizontally centering stuff, the one that I like is to add this
    body {
  height: 100vh; /*if the browser doesnt understand the next line*/
  height: 100dvh; 
  display: grid;
  place-content: center;
}

    accordingly, you dont need such large margins on top and bottom of your .main

    (in fact, these margins break layout for a range of heights).

    • instead of using width, I suggest using max-width. This simple change will make your layout more fluid and responsive to wider range of viewport widths.
    Marked as helpful
    1
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Container width as a % of viewport width looks and feels strange. It's better to just use some fixed values for min-width and/or max-width.

    The same goes for height, only in this case it's better not to assign any value to height, the default in most cases is appropriate. If you try to change the size of the viewport, you'll see the problem.

    (looking deeper, there are a lot of percentage-based heights and widths. I suggest removing them all unless necessary. Since you are using flexbox for many things, you should allow flex containers to decide appropriate widths/heights.)

    Marked as helpful
    0
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Ok, feedback:

    • indentation and consistent code formating is important, really important. Not only does it make reading your code easier, but also is an indicator of overall quality. If you are lazy, you can use automatic formatters like Prettier
    • Don't use <br> tags, unless absolutely necessary. HTML is for structuring your page, not for making some micro style corrections.
    • width: auto doesnt do anything, and width: 100% is in many cases either not needed, or outright harmful. Block elements automatically take up the entire available width space.
    • dont use position: absolute without a good reason. You dont even need it here, just place your text elements below the image. In your solution, this breaks layout for a range of viewport heights.

    So how to fix:

    • first, correct your structure. It should look something like this:
    .container
    .img
    .text
         .title
         .content
    • then, remake your styles to fit the new structure. Dont use magic numbers like top: 67%; left: 50%; Instead, let the layout flow naturally from top to bottom. Utilize CSS box model to tweak positioning of elements.
    Marked as helpful
    0
  • Pavel B.•270
    @JagholinPosted over 1 year ago

    Good job! I only have 1 suggestion:

    • people usually use class selectors, not id selectors in CSS. Here it doesn't really matter, but having class selectors allows for easier reuse of styles(ids have to be unique, classes do not).

    To fix your boundary between the 2 sections all you have to do is move your border-radius and box-shadow properties from #score to main

    Marked as helpful
    0
  • Durgesh•20
    @durgeshrajuSubmitted over 1 year ago

    It's natural to have some areas of uncertainty or ambiguity in code, especially when working on complex projects or using new technologies. In such cases, it's helpful to consult relevant documentation, online resources, or more experienced developers for guidance. It's also a good practice to leave comments in your code to explain any unclear or convoluted logic, making it easier for others to understand and maintain in the future.

    Intro Component with Signup Form - React JS

    1
    Pavel B.•270
    @JagholinPosted over 1 year ago

    Nicely done!

    There isnt really much to criticize, most of the following are just suggestions:

    • I see that you are using create-react-app for your project. It has many problems and is no longer a recommended way to start React projects. For an easy-to-use replacement, I suggest ViteJS
    • Your <label>s for attribute should point to an id of an element being labeled, not to name.
    • I suggest learning these React hooks: useMemo, useCallback and useId. They are easy to use and would be helpful in your app - the first two memoize values and functions, and the third generates unique ids so that you dont have to do it yourself.
    • @import rules in SCSS are deprecated. Its recommended to use @use instead.

    Otherwise, great job!

    0
  • Yiğithan Çolak•90
    @yigithancolakSubmitted over 1 year ago

    -It was a challenging project to build, especially making responsive design was took some time but helped me to sharpen my skills.

    -I think i should have create more re-usable scss components for repeative structures. And i purposely did not change the font-family and added some differences to my UI.

    -If you have any feedback about design and the code, feel free to comment or mail to me.

    Active Steps Form

    2
    Pavel B.•270
    @JagholinPosted over 1 year ago

    Good work

    some things that I noticed in the UI:

    • on the first page, if you dont enter anything and just try to press "next step" button, there are no error messages saying that some fields are required.

    • phone number field doesnt like spaces, even though the placeholder message suggests otherwise.

    • on the second page, the text in panels "arcade" "advanced" "pro" can overflow on some viewport widths(just large enough to still be in the desktop layout)

    • in some circumstances(not sure yet when) the "monthly <-> yearly" switch can break and get stuck in one position.

    • the remaining 2 pages seem to be fine, didnt break :)

    • You dont use any libraries to help with managing state or form validation? impressive. I would recommend Formik or react-hook-form

    • regexes: phone numbers can have 0 after +. Also interesting that you included so many characters in the name regex, but left out "ä". (I dont think you even need name regex, what yre you trying to check?)

    • I see you are using viewport units. Nice! There is this video by Kevin Powell about them that goes into some problems that they might cause sometimes and how to fix them.

    • speaking of CSS, I would add layout breakpoint @media screen and (max-width: 768px) to the variables, because its used everywhere, and is something that needs to be tweaked from time to time

    • didnt even try to make nice circles around step numbers in the sidebar? The easiest way is to just give them definite height and width, like height: 4rem; width: 4rem;

    • you use lets too much, try to use const keyword wherever possible.

    • useEffect on line 91 in file app_context.jsx can be rewritten to grab data from data.jsx about available plans and options and use it in calculation instead of hardcoding a bunch of nested ifs. Also dont put dependent variables like totalPrice into your context, but calculate them on the spot when you need it(and you can use useMemo hook to memoize the value). The principle here is 'make invalid states unrepresentable', in this case, we dont want totalPrice to become desynced from the rest of the state by some mistake.

    • <nav> element is supposed to contain links, but yours doesnt. Also <button> in the navbar is misleading, if it doesnt do anything.

    • in vitejs, you can import an asset like an svg directly into your js file. You dont need to inline their content manually like you do in data.jsx.

    • The "payment plan" switch breaks, because you dont initialize it with correct state from your context in SelectPlan.jsx. This should be something like <IOSSwitch onClick={changePaymentPlan} value={payment_plan} />.

    • You have some accessibility problems(images without alt text, bad tab ordering, and you are using heading levels for styling ). I suggest running accessibility tool in DevTools

    And thats all I could find. Dont be discouraged by all this stuff, your code quality is one of the best I ve seen on the frontendmentor.io so far and is easy to read and understand.

    Marked as helpful
    0
  • Billy•60
    @cscheungaeSubmitted over 1 year ago

    Currently, I have implemented the mobile layout only. It is recommended to view the site while open the chrome inspector mode with mobile view.

    For the UI parts, I mainly used ReactJS for the UI flow control. To implement the logic of the multistep of the forms, I used Redux for state management to store the form data. By installing the redux addons on Chrome, you can view how I manipulate the redux state.

    If I have time, I will complete the desktop layout later.

    Feel free to leave any feedbacks. I would appreciate that.

    Multi-step form with only Mobile Layout implemented (tbc)

    1
    Pavel B.•270
    @JagholinPosted over 1 year ago

    Some things I noticed:

    • error status is not cleared when the user corrects the wrong input, but only when they press the "next step" button. To fix, you can reevaluate error states in onChange events.

    • phone input doesnt [recognize '+'] or spaces. Also [minsize 10] is too large, phone numbers can be way shorter.

    • in slices/formSlice.ts I would add checks that you are not doing out-of-bounds array access.

    • your regexp for phone numbers is way too specific and doesnt allow much room for different formats, and only accepts 10 digits. Also it has an obvious error - to add spaces to a character range [], you have to add slash in front \s. I recommend using a tool like regex101 to handle regular expressions.

    • if you are using <label> with htmlFor, it needs an id of the element that the label refers to, not the name. See https://developer.mozilla.org/.

    • Your checkboxes all have the same id attribute. This is invalid - HTML elements should have unique ids, if any. You can use React's useId() hook to generate unique IDs.

    • On the second page your options dont have any real form elements, (and neither any ARIA attributes) which hurts accessibility. To fix, you can hide an <input type="radio" > in there. This will also help with tab-navigation on this page(or if you want to bruteforce this part, you can set tabindex attribute.

    • Using a translation file for strings is an interesting idea(and one I havent seen here before), but without any UI connected to it its completely invisible to the user. I assume this is something for the future versions of this project.

    • overall it's a good project and shows that you have knowledge about a lot of topics.

    • but I also have a feeling that it's way heavier and more complicated than it should be. Other side of a coin :)

    Marked as helpful
    0
  • Shane Chaffe•900
    @ChaffexdSubmitted over 1 year ago

    This took me a few days and had a few challenges on the way.. I decided to render the components for mobile different to how you might normally do it. I would really appreciate feedback on this!

    REACT + VITE TO DO APP

    1
    Pavel B.•270
    @JagholinPosted over 1 year ago

    Some of the things that I would do differently:

    • you don't really need Sun and Moon components. Since you are using vitejs, it supports importing image assets and you can just set the svg files either as src attribute for an <img> tag, or as background-image CSS property on a div.
    • I find it better to destructure props immediately in the function argument list. Like this:
    const Sun = ({onClick}) => {...}

    This way, it is immediately apparent what props a component expects to receive, and you dont need to read the rest of the function code to find it out. But this is just my preference :)

    • when you have dependent variables like incompleteCount in the ToDo component, you can use useMemo hook instead, and get rid of associated useEffect. Like so:
    const incompleteCount = useMemo(
    () => toDoList.filter(item => !item.complete).length, 
    [toDoList]);
    • newList.splice(index, 0, newList.splice(newList.indexOf(draggedItem), 1)[0]) is a bit hard to read. I would write it out:
    newList.splice(newList.indexOf(draggedItem), 1); 
newList.splice(index, 0, draggedItem);

    • I wouldn't use *.classList.add / remove functions, because they do the same thing as className JSX attribute. When you have multiple ways of doing the same thing, prefer to use that which is more idiomatic to the libraries you use.

    • This is a terrible way of generating "unique" ids: Math.floor(Math.random() * 5000000). Just use natural numbers in order: 0, 1, 2, 3 etc.

    • About your original way of dealing with mobile/desktop layouts... I don't see the point in doing it like this. Not only are you duplicating your code, but your site actually breaks if you resize it (ToDo component is unmounted and all data is lost)

    PS and additional thing:

    • It is good that you use key prop where React requires you to, but it seems that you dont really understand what it supposed to do. key allows React to distinguish components in an array, giving them something to be identified by. index here is not a good key, since it is possible to rearrange todo elements. You already have task.id property, so use it instead.
    <div 
                key={entry.id}
                ...
>
    Marked as helpful
    1