Hassan El Jebyly
@hassaneljebylyAll comments
- @AmirezaMahmoudiSubmitted 7 months ago@hassaneljebylyPosted 7 months ago
congrats on finishing challenge, It looks good, but search and filter by region is not working. you can use the code below to make searching and filtering work, using the states from select and search input as conditions for filtering, then map over the filtered list and display it.
const filteredCountryList = countries.filter((country) => { if ( // filter by region (selectInput === "" || selectInput === country.region.toLowerCase()) && // filter by name country.name.common.toLowerCase().includes(searchInput.toLowerCase()) ) return country; });
Marked as helpful1 - @JIH7Submitted about 1 year ago
I finished this a few weeks ago and just realized I never uploaded it! My goal on this project was to be as pixel perfect to the design as possible. Any feedback is tremendously appreciated!
@hassaneljebylyPosted about 1 year agolooks great, not perfect but great, my advice to you is try and use css custom properties more, even when using scss, you don't want hardcoded values in your main css file, I use scss loops and variables to build
:root
with custom properties and utility classes, that way your main css isn't dependent on sass to be scalable and easy to maintain, one other is it's better to useem
orrem
for margins and paddings in order that typographic integrity is maintained when text is resized by the user, px value are too absolute, also try naming things and avoid nesting selectors on complex pages, that's all I have, if anyone else reading this please correct me if I'm wrong :)0 - @faustocalvinioSubmitted about 1 year ago
any feedback is welcome :)
@hassaneljebylyPosted about 1 year agohey and well done for finishing the project, to answer your question in order to provide assistive technology users with a logical view of the page structure all sectioning elements
<main>
<nav>
<header>
... must have a definedrole=""
attribute, all sectioning elements automatically define one with the exception of the<section>
tag, it only defines region when it has an accessible name using aria-labelledby or aria-label or title. the perfect fix to this in my opinion is to add an<h2 id="last-news-section-title">(title of your choice to describe the section)</h2>
inside that section the addaria-labelledby="last-news-section-title"
attribute to the section, It will be displayed on the page but you can hide it by addingvisually-hidden
class to theh2
, It won't be displayed but assistive technologies will see it.css
.visually-hidden { clip: rect(0 0 0 0); clip-path: inset(50%); height: 1px; overflow: hidden; position: absolute; white-space: nowrap; width: 1px; }
I hope this helps :)
Marked as helpful1 - @incihuseynliSubmitted about 1 year ago@hassaneljebylyPosted about 1 year ago
congrats on finishing this project looks solid and responsive, one thing I noticed is when you click to close the nav then resize to desktop It stays closed, that's because your javascript changes the
style
attribute, instead what I would suggest is writing a class to do the opening and closing of the nav vianav.classList.toggle("open")
nav { transform: translateX(0%); } .open { transform: translateX(200%); }
this way transform is removed from the element. another thing is when you resize the window you can see the nav slide to right when it hits the breakpoint, that's because you have
transition: 0.6s;
changing it totransition: transform 0.6s ease;
solves the issue, or you can just stop animations during window resizing.this might be useful : Responsive navbar tutorial using HTML CSS & JS
good luck :)
Marked as helpful1 - @JIH7Submitted about 1 year ago
This was a really simple one where my goal was to be as exact to the Figma design as possible. Let me know if you see any imperfections!
@hassaneljebylyPosted about 1 year agoCongrats on finishing this, looks neat and solid, just one suggestion put
display: grid; place-items: center;
on the<button>
tag with the star icon (why not a div tho ? a button tag that's not used as a button will just confuse screen readers), plus you may want to addcursor: pointer
on rating buttons, other than that it all looks great.Marked as helpful1 - @Jo-cloud85Submitted over 1 year ago@hassaneljebylyPosted about 1 year ago
congrats, looks like It's just you and me who made the thing flip lol, I like your approach with the animation, just one observation the timer segments don't seem to be responsive, maybe try a clamp on its width, and make everything inside a 100% in width that's how I made it responsive.
Marked as helpful0 - @NawalMalkiSubmitted over 1 year ago
Hi there 👋, I’m Nawal and this is my solution for this challenge.
Any suggestions on how I can improve and reduce unnecessary code are welcome!
@hassaneljebylyPosted over 1 year agocongrats on finishing the challenge, I have few suggestions on each card title make it positioned absolute with the cards positioned relative then do "bottom : 0;" on the titles, this will make it stick to the bottom of the card cause currently it overflows on big screens, one last thing is there can be only one h1 tag on the page: More than one H1 on a page: good or bad? good luck :)
0 - @NawalMalkiSubmitted over 1 year ago
Hi there 👋, I’m Nawal and this is my solution for this challenge.
This was my first time using tailwindcss .
Any suggestions on how I can improve and reduce unnecessary code are welcome!
@hassaneljebylyPosted over 1 year agocongrats on finishing yet another challenge, I have some suggestions and I hope they help, in the "latest articles" section put "width: 100%; object-fit: cover;" on card img tags, it will fix the problem with images not filling the whole width of the card, how ever imo putting a max width on the cards then making img elements take 100% of the width plus adding "aspect-ratio: 1;" will guarantee that your images stay the same width no matter their size, also a "justify-content: space-around; flex-wrap: wrap;" on the div containing the cards will make it more responsive with much less media queries. hopefully my feedback was clear enough... cheers :)
Marked as helpful0 - @hassaneljebylySubmitted over 1 year ago
would love to get some feedback on the javascript
@hassaneljebylyPosted over 1 year agothank you very much for the input, the reason I validate the input before sending the request is to not waste a request on an input I already know It's not valid, there's also a requests limit on the api hence the extreme measure 😅, the map drawing on top of the "result" div is intentional the mobile design is impossible because the result div covers the map, so I made it so it changes z-index on focus, was going to make a better solution tbh but was a bit in a hurry
0 - @Ben-RickettsSubmitted over 1 year ago
really trying to get better at javascript to hopefully move onto learning some react soon. please give me any critical feedback i could use to brush up on my code. Thank you very much
@hassaneljebylyPosted over 1 year agomy suggestion is to create a class for unread notifications instead of specifically selecting by user name, that way your code can be easily scalable example :
css
.unread { background-color: var(--clr-unread); }
js
const unreadNotifications = document.querySelectorAll(".unread"); function changeColorWrapper() { unreadNotifications.forEach((notification)=>{ notification.classList.remove("unread"); }); }
I suggest you check this : building a scalable css architecture
keep building :)
Marked as helpful1 - @unaygneySubmitted over 1 year ago@hassaneljebylyPosted over 1 year ago
congrats looks great, however it needs more input validation, I entered "1" for the number fields and "a" for the name field and it accepted it, I suggest you use regex to identify card company.. etc, here's a useful link for that https://medium.com/hootsuite-engineering/a-comprehensive-guide-to-validating-and-formatting-credit-cards-b9fa63ec7863 good luck.
Marked as helpful0 - @Alaska-999Submitted almost 2 years ago
Adaptive card form with changing state on input. It is written using react, typescript and styled-components
@hassaneljebylyPosted almost 2 years agoHello 👋, Congratulations on completing the challenge! Consider a small improvement:
input { cursor: text; } that section on the left with colorful background image{ background-size: cover; background-repeat: no-repeat; }
also consider using regular expressions to validate form input, this project is interesting I might start it tomorrow, cheers and good luck.
0