Paul Pabilonia• 10
@PaulPabilonia
Submitted
Still learning CSS and HTML. Any recommendation for me to improve? Tips ?
nmorajda
@nmorajda
All comments
- nmorajda• 945
@nmorajda
Posted
-
Use correct indentation in the HTML code
-
Reset default CSS styles for browser (now you have default margin / padding).
-
Change
.container { height: 100%; }on
.container { min-height: 100vh; }and remove the padding, and you will have the element centered vertically as well.
Marked as helpful
0 -
- ThanhVuong0904• 465
@ThanhVuong0904
Submitted
data in (db.json) Hope you like this solution. Please tell me if there is something to improve in my code. Thanks you.
nmorajda• 945@nmorajda
Posted
Links don't work, I guess that's why:
function getData(callback) { const API = "http://localhost:3000/tracking"; ...and correct the errors visible in the report at the top.
Happy codding :)
0 - ThanhVuong0904• 465
@ThanhVuong0904
Submitted
data in (db.json) Hope you like this solution. Please tell me if there is something to improve in my code. Thanks you.
nmorajda• 945@nmorajda
Posted
if(clickIndex === "weekly") { timeFramesCurrent = timeframes.weekly.current; timeFremesPre = timeframes.weekly.previous; } else if (clickIndex === "daily") { timeFramesCurrent = timeframes.daily.current; timeFremesPre = timeframes.daily.previous; } else if (clickIndex === "monthly") { timeFramesCurrent = timeframes.monthly.current; timeFremesPre = timeframes.monthly.previous; }can probably be written shorter and just as legible:
const timeFramesCurrent = timeframes[clickIndex].current; const timeFremesPre = timeframes[clickIndex].previous;or maybe:
const {current, previous} = timeframes[clickIndex]in the second case you have to rename the variables later in the code or
const {current: timeFramesCurrent, previous: timeFremesPre} = timeframes[clickIndex]Marked as helpful
0 - David Kalany• 50
@AmazingCukr
Submitted
Hi all,
this is my 2nd attempt to make this card. I would appreciate any help/criticism to make my code better.
Thanks for all comments!
nmorajda• 945@nmorajda
Posted
Look fine on the desktop, but there are some small bugs in the code:
<div class="Header"> ...Don't use uppercase letters at the beginning of the class name, id, etc.
<div class="text"> ...This div element does nothing.
Why not:
<p class="text"> ....?
It is also not responsive and from a width of 400px a horizontal scroll bar starts to appear.
You also have three errors in the report above.
0 - Segundo Juan• 95
@JSegundo
Submitted
Give me feedback!
nmorajda• 945@nmorajda
Posted
It looks good ... but only on the desktop.
It is not responsive.
Use a media query for this purpose.
Do not specify the height in px (usually the height of the element is to result from its content or the height of the parent element).
If you use the section element, there should be some header inside. In this case, the IMHO section element is replaceable with a div element.
Marked as helpful
1 - Anis• 135
@anisgo
Submitted
Hello Friend - This is my first Project here, If you have any suggestion or tips please write below your feedback is important.
nmorajda• 945@nmorajda
Posted
The height of an element should rather result from its content, and not be fixed:
height: 80vh;Use the min-height property if you must, but you don't have to.
Marked as helpful
0 - Musa• 170
@its-me-musa
Submitted
Any feedback / comments are welcome
nmorajda• 945@nmorajda
Posted
It looks good but ...;)
-
For some resolution range, there is no space left and right on the card.
-
Inside div.stats you have three div elements each with a different class: .companies, .templates., .queries Since their content looks the same and presents related content, it would probably be better to use one of the same name, e.g. .stat
So that in the future, on a different subpage, you can create such an element .stat in a different place or in a different layout.
0 -
- Dharmik_487• 1,740
@Dharmik48
Submitted
Another challenge down✌️. This was a lot of fun and taught me a lot's of cool new stuff! I am really proud of the outcome and it motivates💪 me a lot! Thank you Frontend Mentor for this awesome challenge🤗.
At last if possible, a feedback never hurts. Thank you.
nmorajda• 945@nmorajda
Posted
Why is the color information in the JS file? I guess it should be part of a CSS file and JS only handle event handling.
0 - MaryF• 350
@Janselin
Submitted
Another challenge done! I tried to use more em instead of px this time. It was a fun project. I would love to know all your feedback and tips!
nmorajda• 945@nmorajda
Posted
Good, but IMHO too many div elements:
Example 1:
<div class="info"> <h1>Order Summary</h1> <div class="paragraph"> <p>You can now listen to millions of songs, audiobooks, and podcasts on any device anywhere you like!</p> </div>What's this div.paragraph for?
Example 2:
<div class="payment"> <ul> ...Why not right away:
<ul class="payment"> ...There is also a problem with responsiveness.
However, these are errors / problems that you will definitely deal with.
Happy coding.
Marked as helpful
1 - Bartek• 400
@imxbartus
Submitted
3 working themes full responsive og poppa thats how poppyn
nmorajda• 945@nmorajda
Posted
Cześć :) Wygląda dobrze ale kod JS jest moim zdaniem trochę do przerobienia.
Chodzi mi o to, że zmian wyglądu (theme) powinna być opisana w pliku CSS, a JS tylko obsługiwać zdarzenie i np. dopisywać nazwę klasy w elemencie body. Cała resztę powinien być w pliku CSS.
Na przykład:
::root { --body-bg: #e5e5e5; } .dark { --body-bg: #212121; } body { background: var(--body-bg); }a cały js to coś takiego:
btn.addEventListener('click', () => { document.body.classList.toggle('dark') })Pozdrawiam, N
Marked as helpful
0 - Alba• 95
@koalba
Submitted
I would appreciate any feedback! Please feel free to give constructive criticism!
nmorajda• 945@nmorajda
Posted
Looks fine but keyboard navigation (Tab, Shift + Tab) is not possible because the navigation menu is a bulleted list. Apply buttons or links and the elements will become available.
Marked as helpful
1 - Leonardo Meza• 245
@leonardomeza87
Submitted
My biggest challenge was integrating the animations, let me know if you see something strange on your device 😉
nmorajda• 945@nmorajda
Posted
It looks and works great! Congratulations.
0 @EnzoXavier1001
Submitted
I accept suggestions for improvement
nmorajda• 945@nmorajda
Posted
For mobile devices/small screen, you should use a smaller image file (if I remember it is in the images directory)
Marked as helpful
0- Mathieu FONTAINE• 265
@mathieufontaine
Submitted
Any feedback is welcome :)
nmorajda• 945@nmorajda
Posted
Good job.
If I enter a digit and press DEL (remove this number), the display section changes height.
0.3/6 = the result does not fit on the display.
Marked as helpful
0 - Axseinga• 430
@axseinga
Submitted
Hi guys,
I coded this project using Webpack & Dart sass. For my JS I used JS Classes. For more info please see Readme in my Github portfolio :)
Please let me know if you see any mistakes in my code or anything I can improve I would really appreciate it!
Thank you.
nmorajda• 945@nmorajda
Posted
If I enter a value of 0, I get the text instead of an error message: "$ Infinity"
Marked as helpful
0 - Fraser Watt• 1,790
@fraserwat
Submitted
Nice quick one for a rainy Saturday morning ⛈
If there's anything that has been done in a weird / inefficient / inelegant way let me know - I'm also still getting used to semantic HTML, roles that sort of thing so let me know if that seems right?
nmorajda• 945@nmorajda
Posted
You have built this component based on two independent divs: .illustration__container and .details__container
In my opinion, these two div-s should appear inside some container, e.g. div.card
In the future, this would allow for an easier expansion or use of the component in a different location.
0 - Leon Michalak• 545
@NinjaInShade
Submitted
Any way to improve semantics/accessibility?
nmorajda• 945@nmorajda
Posted
Difficult to navigate with the keyboard (tabulator) because for the .btn class you set outline: none without any additional highlighting.
Marked as helpful
1 - Nikolay Borzunov• 125
@tydusgg
Submitted
Damn, i hate image placing like in first section.
If someone have an example or best practice to place image and background-image easier, i would be very grateful.
nmorajda• 945@nmorajda
Posted
Good job! In the mobile view your menu is visible "by default". It should be visible after clicking.
0 - Shane McElroy• 20
@shanemcelroy
Submitted
If there is any, please provide feedback on all of my document formatting etc. I'm trying to build good habits starting out so that I can carry them with me, whether that's putting DIV's in the right places and labeling classes that make sense.
Also, when positioning my 'about' DIV, I didn't know how to push it up over the bubble image so I used negative margin. After researching a bit, I found that some people consider this a 'hack', so I'd love to know the correct way to position things without diving into negative margin.
Overall, I'm really pleased with what I was able to do and look forward to tackling the next challenge!
nmorajda• 945@nmorajda
Posted
You forgot the border above social-stats ;) Good job.
0 - nmorajda• 945
@nmorajda
Submitted
PageSpeed Insights 91/99 (first check)
Any feedback on what could be improved is welcome!
nmorajda• 945@nmorajda
Posted
Thanks for your comments.
You're right an address shortener must work. So I still have some work to do :)
- Corrected.
- Corrected.
- Errors from the report corrected.
0 - verywebdone• 190
@verywebdone
Submitted
Help Please!!!
The design comparison is not styled. But if you click on the preview it is. Any advise on how to sort it?
Thanks!!!
nmorajda• 945@nmorajda
Posted
There are also no styles in the preview because your directory name is CSS (uppercase) and you have css (lowercase) in your page code.
1 - Bargaouz Saad• 100
@sbrg95
Submitted
Hi, Please give your feedback.
nmorajda• 945@nmorajda
Posted
Personally, this moving cube bothers me. Maybe if it was only pushed back when any questions are expanded and in place when everything is collapsed.
0
