@skeva31
Submitted
Every feedback is welcome. Have a nice day!
@Jagholin
@Walid-Gs
Submitted
desktop first.
Flexbox.
Media queries.
please provide feedback and help me improve 😀.
@Jagholin
Posted
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
Responsive Multi-Step From using Angular. Any feedback is welcome!!!
@Jagholin
Posted
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
Hi there, Your feedbacks and suggestions are much appreciated.
@Jagholin
Posted
Great job!
Some suggestions for the future:
position: relative
doesnt do anything here.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).
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
@mayank1352002
Submitted
@Jagholin
Posted
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
@spacezada
Submitted
Feedback pleaseee! <3
@Jagholin
Posted
Ok, feedback:
<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.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:
.container
.img
.text
.title
.content
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
I couldn't make the "connection" of the two divs in the desktop design. Does anyone know how I can do this? My project is like this It was meant to be
@Jagholin
Posted
Good job! I only have 1 suggestion:
id
s 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
@durgeshraju
Submitted
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.
@Jagholin
Posted
Nicely done!
There isnt really much to criticize, most of the following are just suggestions:
<label>
s for
attribute should point to an id
of an element being labeled, not to name
.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 id
s so that you dont have to do it yourself.@import
rules in SCSS are deprecated. Its recommended to use @use
instead.Otherwise, great job!
@yigithancolak
Submitted
-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.
@Jagholin
Posted
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 let
s 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.
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
@cscheungae
Submitted
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.
@Jagholin
Posted
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
@Chaffexd
Submitted
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!
@Jagholin
Posted
Some of the things that I would do differently:
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.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 :)
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:
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