how can i improve? specially with JS (still need to refactor)
Darko
@dtomicicAll comments
- @escarcanSubmitted over 2 years ago@dtomicicPosted over 2 years ago
First of all I wanna say nice job on the project.
You should set the width of the image in percentage since you've set the first image's width in rem your whole website goes out of viewport on mobile devices (see here) and this is how it looks like when you set the width of it to 100%. On bigger screens you would have to adjust accordingly.
Other than that everything looks nice besides some smaller mistakes but those shouldn't bother your. Good job and keep it up 👍
Marked as helpful0 - @khalilnazariSubmitted over 2 years ago
Dear community members,
This is the second challenge. I have developed the interactive rating component. Please review the code in the repository and provide possible feedback on my code structure and readability.
Thanks & regards,
Khalil
@dtomicicPosted over 2 years agoCongrats on finishing your second project.
I've taken a look at your project and everything looks good except a few things with your JS.
First you should avoid changing the style the way you did with
element.style.display = "none"
, instead what you should do is make a class in CSS (for example class show) and then you toggle that class to an element through JS like this:CSS
.element { display: none; } .show { display: flex; }
JS
element.classList.toggle("show");
So now when you click the submit button your element will go from
display:none;
todisplay:flex;
and if you click the submit button again (which isn't possible in this design but I'm telling you for future reference) the element would have an attributedisplay:none;
so basically you're toggling between two states. You can read more about it in depth hereAlso another thing I've found is that no matter what rating you pick in the end screen it always show you picked out 5/5 since you've hardcoded that part, what I would suggest is make on submit check which button was selected (was it the one with the rating 1, rating 2 etc.) and then depending on that set the rating on the end screen (you can do that with innerHtml and some if/else statement).
And the last thing I would recommend is to seperate your JS from your HTML meaning you should write JS in it's own file no matter how little of JS code you have.
Once again congrats and keep on learning 👍
Marked as helpful0 - @Pomz010Submitted over 2 years ago
I want to prevent input fields to accept zero as first character, here's what I have tried. I used addEventListener with keydown event to detect type of character, then store the key (e.key) in an array container and access that character using array index number. If the character is zero, then it will return a returnValue of false (e.returnValue = false) to prevent zero from being typed in the input field. This works but if user made a mistake and deletes characters using backspace, then the input field will now be able to accept zero as first character again.
@dtomicicPosted over 2 years agoHey, I don't know if you are done with this project or just posting for help but if you're done you might consider looking into why none of the buttons work (they don't calculate anything). But since I'm guessing you're still not done and you're just asking for help you can check out this link for the solution to your problem, it's simpler than what you've done, it checks on keyup if the character that was inserted has a length of 0 (meaning it's the first character) and then checks the ASCII code if it matches 48 (which is 0 in ASCII table).
Hope this helps a bit, design looks good you just miss the functionality and you'll be all set. Keep it up
0 - @JunasVeeSubmitted over 2 years ago
Not sure why the image doesn't show up. Does anyone know why?
@dtomicicPosted over 2 years agoCongrats on finishing the project, the picture shows up for me (if you're talking why there isn't a picture in the design comparison that sometimes messes up due to paths and stuff like that but no need to worry that's why there is a live preview of the site) as for the other stuff everything looks pretty nice, the card and everything else is responsive and looks good on small and big screens.
Only thing I would recommend is to avoid adding
<br>
tags when you're trying to break up text (here) because when you get to bigger screens and the text naturally goes into a new line it will break up on the<br>
tag and wont look nice after (this is how it looks on bigger screens). What I would recommend is to set the width of the text container or font-size until the text breaks into new line naturally and then you'll get a nice look and can easily adjust it.Overall good job 👍
1 - @Noke0Submitted over 2 years ago
I found it difficult to read the JSON file in JavaScript but it turns out you just have to host it as an JSON server (With the library OFC or you could host it on github). When it comes to how the website scales I am not sure if I did it the best way
@dtomicicPosted over 2 years agoHey, congrats on finishing your projects, just wanted to let you know that it is not necessary to host the JSON file anywhere you can read it from your folders (like I did here). As for the scaling of the page it's not perfect but it looks quite good, a little bit of work on some stuff and you should be done, but overall it looks good and what's most important it functions.
Great job keep it up 👍
0 - @Azi-01Submitted over 2 years ago
Hi, this is the solution that I came up with for this multipage website. I had a really hard time building it with all the different components which are a part of this site. The css I wrote is a mess and there is a lot of redundant code, so I know it can be improved using best practices. There are some other issues with the site, that unfortunately I couldn't figure out how to solve and I ran out of time. Please give your feedback, it'll be highly appreciated. Thanks.
@dtomicicPosted over 2 years agoCongrats on doing the challenge, I've just done it myself and I can confirm that it is a bit harder so no worries if you didn't manage to make everything look exactly as the design, I don't know if you followed the provided design in Figma and/or Sketch but you should have exact font sizes, margins etc. in there so you should check it out (since I see some of your stuff is a bit bigger than it should be).
Other than that as I said no need to be worried about little details you can always go and fix problems one by one.
Good job 👍
1 - @NavadeepkpSubmitted over 2 years ago
am failed to create hover effect to the image, i tried so many time , but i can't complete it..! am just posting my half finished projects...! am trying to figure out the hover effect , in future i will add to this..!
visit my code and Someone help me to write code more simply & neatly..! Your reviews and suggestions make my moves easier...! give me some tips and suggestions to do better code. am trying to figure out to learn javascript but for me its kinda hard to learn javascript can anyone have time to be my mentor and give a right path..?!
happy coding...!
@dtomicicPosted over 2 years agoHey Navadeep, I've looked at your solution and saw that you're doing the hover effect on the text through JavaScript, which is not a good way to do it since it's not really needed as CSS offer a selector for hovering (reference 1, reference 2) you just add the selector in your CSS for the element you want to have an effect on and that's it. No need for JavaScript. Also it's not really a good practice to change the style of an element through JavaScript if it's not really needed as you can usually do all of the stuff through CSS.
Also I've seen that you position your elements with relative and absolute positions which you shouldn't use for normal positioning, you can use flex for them (flexbox), I'm talking about this section here, it would've been much easier for you to have used flexbox there.
I noticed you also used min-width on your card which is not really a good way to do that, you should always try to position your elements inside of the card and then just give the card a padding and it would look nice.
As for JavaScript there are a lot of materials online but this is a good start.
Keep up the good work 👍
Marked as helpful1 - @Darko96Submitted over 2 years ago
Any feedback and suggestions on how I can improve are very welcome! 😊 Thanks
@dtomicicPosted over 2 years agoHey, congrats on finishing the challenge. I've taken a look and found some issues you might try to fix.
I see you're using multiple breakpoints which are probably doing you more harm than good. What I would recommend you for your future projects is to start going mobile first since it's a lot easier that way to scale up the site later rather than to scale it down. And on the plus side you wouldn't need that much breakpoints you would build your site for 375px and then you could give it a first breakpoint at like 768px for tablets and in most cases you won't need any more(with these kind of easy projects, later on, on more complicated projects you will need to use more breakpoints).
Because as you can see now here your elements on mobile screen are going outside of the viewport and that's bad, you could try fixing that with setting max-width on some of your elements. Also I would recommend you to do some more research on flex and utilize it more since it's a great thing.
And also here your website completely breaks apart and all of the stuff goes everywhere, that's why mobile-first is easier and better especially for beginners (I don't know if you did this project mobile-first but if I would have to guess I would say you didn't).
I would recommend you to play around with your code to try and figure out what you missed out and fix it, one thing I found useful when I was just beginning and a thing I do still is when writing code try all of the different styles and that way you'll understand how elements react to different styles and how they position themselves and other stuff.
On desktop the site looks good and true to the design, keep it up 👍
Marked as helpful1 - @MouedrhiriSubmitted over 2 years ago
Challenge accepted : Advice generator APP The only difficulty I had is to find the lighten green color so for the ones who will try to do his challenge here is the code : on Hover : box-shadow: 0px 0px 20px #53ffaa;
@dtomicicPosted over 2 years agoOn my project I just used the neon green color that was assigned in the project and made the box shadow with it but looks pretty much the same haha.
You should consider adding the font that was provided by Frontend Mentor (Manrope) and not Rubik. I don't know if you are aware but when you download the starter files you get a
style.md
file where you can find fonts and colors used. So that might be helpful.Anyways good job, keep it up 👍
1 - @KalpChaudharySubmitted over 2 years ago
Here's my site. I have done almost every task which was given . Except , mobile responsive. Sorry for that :\ but the rest of the stuff is just working excellent. Thanks :)
@dtomicicPosted over 2 years agoHey Kalp, I've looked at your solution and found some things you could improve.
First of all when you're at the top of the page and click bookmark it refreshes the page, same thing with select the reward, when you click it it just refreshes the page and sometimes the back this project button doesen't open the modal until you scroll down. That sounds to me like a JavaScript issue and you should look into that.
Next thing I stumbled upon is when the user donates the $ sign goes away from your total amount donated (image). Also when the donations go over a 100000 your progress bar goes out (picture), you can fix that by setting a max width on the progress bar fill so it doesen't go out even if the donations pass the 100000 mark. Next thing with donations is that you can input the negative numbers and then your donations go down (try donating -80000 and you'll see) you can fix that by just setting that the donation amount can't be less than zero.
Mahogany reward should be grayed out and disabled while yours isn't.
As for the modal you don't have any hover effects on the text and also a user can check all the boxes which I think shouldn't be possible (picture).
Hope this helps you solve some of the problems and issues you have, you could also look into doing the mobile design properly (I would suggest for your future projects to do mobile-first design since it will be easier for you to scale it up later rather than having to scale it down).
Other than that great job, keep it up 👍
Marked as helpful1 - @muben88Submitted over 2 years ago
Hello, Frontend Mentor people! Hope you're doing great! This is my solution for the Tip calculator app challenge. the sliding bar was a good challenge! you should try it out.
Happy to hear any feedback and advice.
@dtomicicPosted over 2 years agoI've taken a look and it looks great, you've managed to do the stuff that has been bothering me in my project (the slider fill color), looking at it everything looks fine besides the yearly billing on mobile devices (here) they should go in a row next to each other so you could look into fixing that.
Also what I noticed is that you've set the values yourself for pageviews and costs but Frontend Mentor already provided them in the
README.md
file (here) so you could set it that way you only have 6 values and you can put like when the slider value is below 20 have the first value then between 20 and 40 second value and so on.Otherwise great project, keep it up 👍
Marked as helpful1 - @dyntbnSubmitted over 2 years ago
Hi everyone, 👋
Happy with how the responsiveness came together in this project. However, I came to wonder what "real world" media queries are targeted in a professional project. 🤔
My current method I'm going by is targeting
1400px
and375px
. In between those targets, I've developed a pattern of targeting:900px
,700px
or800px
, based on where it breaks in these challenges.Is my method normal (fix it as it "breaks")? Should I standardize my approach by targeting specific sizes? For example, common screen sizes mentioned by a blog.
Would really like to hear your thoughts. Thank you for reading, have a great weekend! 🙂
@dtomicicPosted over 2 years agoI would have to agree with the comment above, I always start mobile-first and it's so much easier for me since I end up only using one or two breakpoints (usually just 768px but sometimes also 1440px) if it's not a larger project then more breakpoints are of course useful. But for small project like this I think you could've easily pulled it off going mobile first from 375px and then make the card for that dimension and give it a max-width and then on 768px you just convert it into another style and that's it.
Writing mobile first usually makes your job a lot easier and less frustrating.
Great job, listen to the advices in the comments above and keep on learning 😊
Marked as helpful1