@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 useconst
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 liketotalPrice
into your context, but calculate them on the spot when you need it(and you can useuseMemo
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
@yigithancolak
Posted
Wow go easy on me i just doing this for 4 month :)) First of all thank you for looking at my code very detailed and being so kind to not discourage me. It was a very good vision. I know my app has to be more UX friendly i will improve that later on. About the switch break.. I dont now why this happening but i will try to give it a value but the value i was changing is a string it only can be 'monthly' or 'yearly'. I dont now how convert it to true or false but we will see. And also big thank you for the svg thing it was the first time i was dealing with svg's. Can you give me a example of importing it ? @Jagholin
@Jagholin
Posted
@yigithancolak You can convert "yearly" or "monthly" value to a boolean by comparing it to another string. Like payment_plan === 'yearly'
for example.
With vitejs, you can import image files in your sources, like for example import myIcon from '../assets/icon.svg'
. In this case, myIcon
will contain an URL of the image by default, which you can then assign for example to a source attribute of an <img>
:
<img src={myIcon} alt="" />
. Dont forget to add alt
attributes, the empty string here means that the image doesnt have any special meaning and is just a cosmetic element.
Marked as helpful
@yigithancolak
Posted
Also do you think the folder structure is good ?
@Jagholin
@Jagholin
Posted
@yigithancolak its fine. Personally I also like to separate components into "pages" and "controls"(things like buttons, input boxes etc), but I dont think its a requirement
Marked as helpful
@yigithancolak
Posted
Thank you for the feedback have a good day @Jagholin