Skip to content
OverviewSolutions
5
Comments
14
Amélie
@aweliego

All comments

  • Amélie•330
    @aweliego

    Hi Pex-Dev, and good job on finishing this challenge! I recently completed myself so I know it wasn't easy 😄

    I can't speak much about the code itself because my vanilla Javascript skills are a little bit rusty so my feedback is more about my user experience of your solution. The responsiveness of the layout is really good and overall the app looks good with everything in the right place.

    As for the functionalities, most of them work as expected, I just noticed two things when changing the score of a comment:

    • the current user is able to upvote/downvote their own comment, while this shouldn't be the case
    • when you click twice on the + or - icon, on the second time the score resets to the previous value; I'm not sure this was intentional from you, I found it a bit odd from a user perspective

    Overall really well done, keep at it!

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi Copper, and congrats for completing the challenge 🎉

    I wanted to try and help you with the problems you mentioned:

    • Regarding the issue on small screens with the search bar overlapping on the listings when there are many filters: currently in your App component, you call the Filter component inside the background div. If you call it just after the div instead, the issue you mentioned doesn't exist anymore. Of course you will need to adjust some styles in your Filter component (like removing the absolute position) to make it look good, but just to give you a starting point :)

    • Regarding the position of the company logo on small screens: on small screens you could set the logo in an absolute position (inside your div which is already relative) and adjust -translate-y until it's in the right location. Then on medium sized screens, override these styles with the static position and reset -translate-y to default. You will also need to adjust the spacing of the text content around and make the image smaller but I hope this gives you an idea of how you can achieve it :)

    I hope this helps and wish you good luck! 🔥

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi Azar, well done on this challenge! 👏

    Unlike most solutions I've seen, yours is quite interesting because the user gets the results that match exactly all the selected filters (some if I filter on 'CSS' I get 2 results but if I add 'Senior' to that I get only one, while in other solutions, including mine, you still see 2 results). My filtering function is very similar to yours but I see that it's because you used the every array method while I used some. So nice to notice these little differences that have different effects!

    What you came up with to remove the filter blew me away, I would have never thought of doing it like that. Will definitely make a note of this! ✍️

    If I had a critic it would be about the overall structure and how the components fit together. To me it's a little bit tricky to comprehend. The component names could in my opinion reflect more what their role/content is, for example I would have chosen the name Search over Filter (a bit too generic given how much the word filter can be used through this project); JobCard and JobListing also sound too much of the same to me. But finding good names for each component of this project wasn't easy for me either, especially when it comes to the filter buttons inside the job listings and the 'tags' in the search bar.

    I think you did a great job and I learned from your solution! Good luck with your future projects! :)

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi! Well done on completing the challenge, it looks good!

    I took a look at your code too and had some tips in case you're interested in refactoring your code!

    • To avoid using a useEffect to set the correct background for desktop or mobile, since you're using TailwindCSS, you could handle this via the tailwind.config.cjs file:
    module.exports = {
  content: ['./index.html', './src/**/*.{js,ts,jsx,tsx}'],
  theme: {
    extend: {
      backgroundImage: {
        'header-desktop': "url('/public/assets/images/bg-header-desktop.svg')",
        'header-mobile': "url('/public/assets/images/bg-header-mobile.svg')",
      },

    And then apply the classes bg-header-mobile sm:bg-header-desktop to your div. That way your code is 'cheaper' and drier :)

    • In your JobListItem component you repeat 4 times the same button for each filter type. You could refactor this by putting all the filters in an array and mapping over this array to return the button. Something like that:
    {[job.role, job.level, ...job.languages, ...job.tools].map((filter, idx) =>
<button key={idx} onClick={() => handleFilterClick(filter)}>{filter}</button>)}

    Hope that's useful. Happy coding!

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi! I was looking for more solutions on this challenge and came across yours.

    I personally like using flexbox for elements like a navbar as I find it more suitable than grid for small components (flexbox being content-first based while grid is layout-first based), but in principle you can use whichever you like to achieve the same thing. I think you did well with flexbox!

    It's been a minute since I wrote vanilla JS but I don't think you can do much shorter/better for the dropdown functionality (and the other functionalities for that matter). Your JS does exactly what it's supposed to do and is concise. I like that it's simple and easy to understand!

    Regarding the height of the image and description, I assume what you want to do is having the bottom of the description (the client images) aligned with the bottom of the image. If that's what you meant, you can achieve this with align-tems: end; on the hero container:

    .hero {
  margin-bottom: 3rem;

  @include breakpoint-up(xlarge) {
    display: grid;
    grid-template-columns: 1fr 1fr;
    grid-auto-rows: auto;
    gap: 2rem;
    width: 90%;
    max-width: 1000px;
    margin-right: auto;
    margin-left: auto;
    align-items: end; ------> add this
  }

  &__image {
    margin-bottom: 2rem; ------> remove this
    order: 1;
  }

  &__description {
    padding-right: 1rem;
    padding-left: 1rem;
    text-align: center;

    @include breakpoint-up(medium) {
      padding-right: 3rem;
      padding-left: 3rem;
    }

    @include breakpoint-up(xlarge) {
      text-align: left;
      align-self: center; ------> remove this
      padding: 0;
    }
  }

    Or just replace align-self: center; with align-self: end; but I think it's nicer to have the property on the parent element. Then you just need to play with the margin/padding of the text elements inside the description to make it look better.

    Regarding the breakpoints, I usually go for just two or three views of the site, so mobile/desktop or mobile/tablet/desktop (depending on the project). I agree that it's a bit overkill otherwise to have so many, and most sites will render well also with fewer breakpoints.

    Overall I think you did a good job on this challenge, your code is very clean and easily readable, and the responsiveness is top notch! Keep it up!

    EDIT: something I forgot to mention - I find it a bit unhandy that the drop-downs in the navbar open on hover but that you need to click the button to keep them open, and that to close them you need to click again exactly on the button (instead of just anywhere). So I think there could be some improvements here as well. Also, the arrow icons (both in the navbar and sidebar) are not pointing to different directions when you open/close the drop-downs, as shown on the designs.

    Marked as helpful
  • Amélie•330
    @aweliego

    😍 What a brilliant and inspiring solution to this challenge!

    I really love how you structured your project and that you really nailed the accessibility (something I need to work on). The transitions are really smooth and the dark mode is a great addition!

    The only thing I noticed with the dark mode is that if I leave the site after switching to the dark theme, and then come back again, there is a slight delay for the dark theme to load, so it goes from light to dark really quick. But maybe that's something that you were able to solve with the advice in the first comment?

    The responsiveness is top notch, although the layout could be a bit improved on tablet views imo. But I think you have some real styling chops here! I'm still trying to wrap my mind on how you handle the arrow icons and the direction they point at to be honest 🤯

    And my last suggestion to make your solution even better would be to render the data more dynamically, as currently much of the menu text is hard coded. You do map over the menu items to create the links, but I would take it a step further and extract the data to a separate array, and refer to it where needed in your code. That way it's all easy to find in one place, and easier to maintain as well in the future should the menu grow.

    But fabulous job overall, and I bookmarked your solution for future reference!

    Marked as helpful
  • Amélie•330
    @aweliego

    Really nice take on this challenge. I was not familiar with web components so your solution allowed me to get acquainted with a new concept. Because I've never used them or seen them being used before, I don't fully understand all the code of your project, but I'm quite impressed with how robust and neat it looks. The responsiveness is absolutely fantastic and the animations are a great addition. Beau travail !

  • Amélie•330
    @aweliego

    Awesome solution. Your code is very clean and feels pretty advanced. It's really interesting to see all the different ways one can come up with to switch between the timeframe, yours in particular seems to effortlessly do the job. I'm happy to see TypeScript/React solutions on this project, as I'm still struggling to see how to use TypeScript in general. I'll bookmark your solution for future reference, if you don't mind.

    The only thing that I thought could be improved is the responsiveness. When I reduce the browser size there's a point where the cards slightly overlap each other, and scrollbars appear (something to do with the overflow and/or width/margin I guess).

    Great take on this challenge!

    Marked as helpful
  • Amélie•330
    @aweliego

    Wow! Nice solution! I also used React in this project so I was curious to see how others completed it with React. It's funny how it's totally different than my solution but I kind of like that it's compact and efficient. The way you rendered the stats dynamically is great. The SCSS looks pretty good too, I would have never though of using a linear-gradient for setting the background icon. For all these reasons, bookmarking your solution :)

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi there,

    Well done completing this project! 💪

    The desktop view looks good to me, while the mobile version seems to have too much height, due to the content being stacked vertically instead of occupying the space horizontally. Nice that you use grid areas, the CSS looks pretty neat!

    Regarding how to keep the selected timeframe active: you can use a CSS class for this, that you can add to the timeframe when it's clicked. You can accomplish this using a loop (forEach or for loop). You will also need some code to first remove all active classes when clicking on a timeframe so that only one timeframe remains active at a time. I would also suggest giving that active class to one of the timeframe by default so the user knows which stats are being displayed upon opening the app.

    Changing hrs to hr: you could use an inline ternary statement to reduce the amount of code here, something like ${value === 1 ? 'hr' : 'hrs'}

    As for the general organisation of the code and the repetition pattern:

    • HTML: a lot of divs there, I would make this more semantic; to reduce the amount of HTML I would also create the card elements dynamically through JavaScript instead of hard coding them (though it makes sense you first harde coded them to style them!)
    • If you prefer to have the elements hard coded in your HTML, I would definitely advise to store them in variables to make your code more readable. You can namely store the timeframes and cards in variables with .querySelectorAll() (but you'll need a container for the cards in your HTML for that) and then work through the functionality using a loop to avoid the repetition.
    • You will also probably need to iterate through the JSON object to get the correct values; mind you, I can't be more specific here because I completed the project in React which made this part much less complicated than if written in vanilla JS.

    I understand this is one of your first projects in JavaScript, so good job on making it work and keep at it! The points I highlighted above are pretty common techniques in everyday JavaScript that I use in almost every vanilla JavaScript project.

    Hope this helped and happy coding! 🔥

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi John,

    Well, from what I can see your code looks very neat, DRY and accessible, so well done! Regarding what you mentioned about struggling to write 'simple code': totally normal when you're starting out. I would say, don't worry about your code looking very efficient and DRY at the beginning, just focus on making the app work. Once you have that done, you can focus on refactoring and it should be easier then to see what you can bundle together or separate into different functions etc. It takes a lot of trial and error and there are also so many ways you could have built this app with vanilla JS, there's not just one right way to do it. Anyway keep at it, the more practice the easier you will find it to get into the implementation of the features :)

    Nice that you used Sass (you should add it to your readme!) and I actually thought it was quite creative to increment the index of the score by 1 to get the right value. Also great that you thought of the disabled attribute for the submit button.

    Design-wise, your solution looks a bit more 'compact' than the original, I would perhaps add some spacing/padding.

    Regarding your JS, just two small comments:

    • There's a bit of a mix of ES6 and older syntax in your code, I would standardise everything to arrow functions
    • Instead of naming your variable rateBTN, I would name it rateBtns - it's best practice to use camelCase and since this refers to multiple buttons, it would make your code more readable to make it plural :)

    Happy coding!

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi Benny,

    There are many, many ways to accomplish this with vanilla JavaScript and CSS. Part of what one might consider a solution more correct than another might be the side effects caused by the chosen method. Mostly, with the 'position absolute' method (which I've also used, btw) is that the element with position absolute will remain visible in the DOM, but this harms accessibility as absolute positioning overrides the default flow layout of browsers. From what I understood, this would also be the case if you would try to tweak the visibility/opacity or use transform properties (such as translate() or scale()) to make the element disappear from sight. [Please someone correct me if I'm wrong]. From what I've found, only changing the display from block to none will effectively remove the element from the DOM and thereby provide a better experience for all users. The WebAIM provides a good thought starting point on the topic: https://webaim.org/techniques/css/invisiblecontent/

    That being said. Targeting the display property won't allow for CSS transitions (in case you'd want to add some to switch from one card to another) as it is not possible to animate/transition from display: block to display: none. Therefore I found it is tricky to accommodate accessibility and transitions, but I'm sure there are ways to do this, just haven't looked for them at this point.

    Regarding your solution for this particular point: I'm not sure about the necessity to position the rating tool card absolute. If I remove that property in the dev tools, it seems to be hiding the attribution div, but if you delete that one you have no need for the absolute position anymore.

    Some more feedback:

    • I think the colour scheme for the buttons on hover/click is reversed: they should actually be greyed out when you hover over them and the one that is click should change to (and remain) orange :)
    • I would make the cursor a pointer when hovering over the rating and submit buttons

    Other than that, your code looks quite DRY and you used both Grids and Flexbox which is interesting - good job on this challenge!

  • Amélie•330
    @aweliego

    Hi Thobo!

    Good effort on that challenge!

    Regarding your question about changing the styles of the ratings when clicked: the method I always find the most straight-forward is to create an .active class in your CSS (which would make the ratings orange) and then iterate over the ratings in your JS and add the active class when the rating is clicked. Keep in mind you'll also have to find a way to remove that .active class if a user decides to select a different rating.

    I would also change the #submit:active selector to #submit:hover to be able to see the styles of the button change, as otherwise the change in styles happens so fast (the button only stays on its active state for a few milliseconds) that it is not really noticeable.

    Regarding your HTML, you might want to go with a more semantic approach - for example use a <button> element instead of a <div> for the Submit button and an <img> element for the images :)

    Hope that helps!

    Marked as helpful
  • Amélie•330
    @aweliego

    Hi Kyle!

    I can't speak to Tailwind, but as far as I can tell, you did a good job building this app with React! I also liked that project, very simple in appearance but makes you revisit a lot of frequently used functionalities indeed. Like you, I found it a bit frustrating to see the difference between the dimensions of my solution and those of the design after submitting my solution, but then again, it is in my opinion not crucial to the quality of the project.

    I also enjoyed reading your README by the way. It was interesting to read your thought process about the challenge and what you learned :)

    Upon testing your solution I wanted to share with you the following suggestions/feedback:

    • I would give the 'cursor' property a value of 'pointer' for the rating and submit buttons, so it's clear that these are clickable elements.
    • If I submit without having selected a score, I get a 'You selected 0 out of 5', which makes sense since the rating state is initialised with 0. I suppose this is not the intended behaviour, so it would be nice to have some logic to prevent that from happening. [I am totally cheating a bit here, because I also just submitted my solution on this challenge yesterday and someone pointed out to me that my rating is 'undefined' when clicking Submit without selecting a rating - so I'm just passing this very good feedback around I guess 😆]

    And as part of your continued development, why not add some transition when switching from the rating component to the thank you card? :)

    Well done again! 👏