Skip to content
OverviewSolutions
4
Comments
36
P
Christopher Adolphe
@christopher-adolphe

All comments

  • Christopher Adolpheβ€’600
    @christopher-adolphePosted 25 days ago

    Hi @tinuola πŸ‘‹

    You did a great job on this challenge. πŸ‘ I had a quick look the code base and here's small feedback:

    • You could simplify the interface of the ProductCard.vue component by declaring only a product props instead of each field of a product as follows:
    defineProps({
  product: Product;
});

    This will enhance readability.

    <ProductCard
  v-for="product in products"
  :key="product.name"
  :class="{ selected: product.selected }"
  :product="product"
/>

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Samuelβ€’840
    @Samuel-AmaroSubmitted over 1 year ago

    This project was very challenging from the start, it took longer than I had estimated, the biggest challenges were building accessible components with focus management, in addition to the data structure for the client-only boards, there were many challenges, which I encountered when trying to build this project, however it was gratifying to see the result, feel free to evaluate the code and give me tips on what I can improve.

    kanban web app with context api and reducers with typescript and immer

    1
    Christopher Adolpheβ€’600
    @christopher-adolphePosted over 1 year ago

    Hi @Samuel-Amaro πŸ‘‹

    You did a great job on this challenge. πŸ‘ I had a quick look at the application as well as the code base and here's my feedback:

    • The buttons for the dropdown menus are too small (4px by 20px) and as a result, this reduces the both touch and click accuracy. For better user experience, the recommended size of icon buttons should be in a range of 42 to 72px.
    • I think the Content component file contains too many sub components. You could have moved ColumnBoard, TaskList and CardTask in dedicated component files under the components directory. These components indeed compose your content but each of them have different responsibilities which is why I think good to separate them. However, this one is only a personal preference πŸ˜‰ I know React gives you the freedom of organising your folder structure any way you want.

    I hope this helps.

    Keep it up.

    Marked as helpful
    0
  • Christopher Adolpheβ€’600
    @christopher-adolphePosted over 1 year ago

    Hi @fontainm πŸ‘‹

    You did a great job on this challenge. πŸ‘

    I had a look at your code and here are a few things that might help you with your class names and the structure of your markup:

    • I get the impression you tried to use the BEM methodology to write your CSS but you did not applied it 100%. You have defined .notifications as a Block which has the .notifications__header and .notifications__list as its Elements. This is good πŸ‘. However, the .notifications Block has other Elements inside it which you chose to target differently. To be honest, it's not a bad thing as such but if you'd like to go the full BEM way, here's a suggestion for the class names and markup:
    <div class="notifications">
  <header class="notifications__header">
    <h1 class="notifications__title">Notifications</h1>
    <span class="notifications__count">{{ countUnread }}</span>
    <button type="button" class="notifications__btn">Mark all as read</button>
  </header>

  <ul class="notifications__list">
    <li class="notifications__item">
      <img class="notifications__avatar"/>

      <div class="notifications__content">
        <div class="notifications__sender"></div>

        <span class="notifications__time"></span>

        <div class="notifications__body"></div>
      </div>
    </li>
  </ul>
</div>

    With the above markup, you will end up with the following CSS:

    .notifications { ... }

.notifications__header { ... }

.notifications__title { ... }

.notifications__count { ... }

.notifications__btn { ... }

.notifications__list { ... }

.notifications__item { ... }

.notifications__item--unread { ... } // This will be your `Modifier` to style an unread item

.notifications__avatar { ... }

.notifications__content { ... }

.notifications__sender { ... }

.notifications__time { ... }

.notifications__body { ... }

    As you can see, if you follow the BEM methodology, your final CSS is completely flat, i.e you'll not have selectors like .notifications__list .notification .message { ... }

    • One last thing, use semantic elements where needed. For example, the Mark all as read should be a <button> instead of a <div>

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Abed Fetratβ€’450
    @abedfetratSubmitted almost 2 years ago

    This is another of my projects built with React. I used Styled components for the styling. For the drag to reorder function I was thinking of implementing it without any libraries, but after doing some research I found out that the native drag and drop API didn't work with touch screen devices. So I ended up using Framer Motion because of it and that we get nice animations for free.

    Let me know If you have coded the drag and drop functionality without using libraries, as I'm still interested to see how it's done. ✌

    Todo app built with React | Styled Components | Framer Motion

    1
    Christopher Adolpheβ€’600
    @christopher-adolphePosted almost 2 years ago

    Hi @abedfetrat πŸ‘‹

    You did a great job on this challenge. πŸ‘

    One small thing I have noticed, given I have added 3 new tasks, when I drag the last task from the bottom of the list and drop it at the top, it gets marked as completed. I'm pretty sure this has something to do with the region of the task that's clickable to toggle as active/completed. If you initiate the dragging from this region, when you'll drop the item, the toggling is also triggered. But strangely, I notice it is only happening when dragging an item from the bottom to the top.

    Framer Motion looks really cool. I want to try it on my next project. I've always used CSS modules with React but when I see how clean you've been able to keep those styles within your components, I think I should styled components a try as well.

    Keep it up.

    Marked as helpful
    0
  • Christopher Adolpheβ€’600
    @christopher-adolphePosted almost 2 years ago

    Hi @MuriWolf πŸ‘‹

    You did a great job on this challenge. πŸ‘ The markup, styling and component composition are well done. πŸ‘Œ

    One easy fix to get rid of these accessibility report errors/warnings is to set a value to the lang attribute of the <html> tag like so <html lang="en"> in your public directory.

    I have been trying to do various actions in the application and I have noticed that when I reply to a comment, my reply is not added right away. I had to add another reply then both of them appeared. I'm not sure if it's because your REST API is hosted on Heroku and it is responding slowly but you might want to double check that as it hinders the user experience.

    In case it's your REST API which is responding slowly, there is a way you could give a faster feedback to the users. Right now, you've written the CRUD operations of your component in a pessimistic update style, meaning; you are making the calls to the REST API and then you are updating the UI. You may give the user the impression that application is faster by doing the opposite, i.e; you will update the UI first and then make the calls to REST API. This is an optimistic update. πŸ˜‰ However, when doing so, you will need to keep a reference of the previous state so that if ever an error occurs while doing the update, you can safely revert to that previous state. Below is an example of what an optimistic update looks like in code:

    // 1)Keeping a reference of previous state
const previousComments = [ ...this.comments ];

// 2) Updating the UI
this.comments = [ ...this.comments, this.newComment ];

try {
  // 3) Calling the endpoint to post the new comment
  const response = await fetch('some-endpoint-url', this.newComment);
} catch (error) {
  // 4) Reverting to the previous state if an error occurs
  this.comments = [ ...previousComments ];
  // 5) Giving a feedback that an error occurred
  alert(`Sorry, an error occurred while adding new comment: ${errror}`);
}

    The above code snippet is not Vue.js specific but I think you'll be able to implement it somehow.

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Christopher Adolpheβ€’600
    @christopher-adolphePosted about 2 years ago

    Hi @Shady-Omar πŸ‘‹

    You did a great job on this challenge. πŸ‘ Here are a few things that I have noticed and that you might want to check in order to improve your solution.

    HTML:

    While the page you built is good, you could improve it by using HTML semantic elements in the following ways:

    • For the navigation, you could wrap the <ul> element in a <nav> element like this:
    <nav>
  <ul>
    <li>
        <a href="#">Home</a>
    </li>
    <li>
        <a href="#">New</a>
    </li>
    <li>
        <a href="#">Popular</a>
    </li>
    <li>
        <a href="#">Trending</a>
    </li>
    <li>
        <a href="#">Categories</a>
    </li>
  </ul>
<nav>
    • The <a> element is also missing for your navigation.
    • Try as much as possible to refrain from duplicating HTML elements on your page because this hinders the maintainability of your code. You can use the same markup for the navigation on both mobile and desktop and adapt the styles by using CSS media queries.
    • You could replace the <div class="top-content"> and <div class="bottom-content"> by a <section> element.
    • For the "New" section, you could replace the <div class="top-right"> as well as it children <div class="text"> elements by an <article> like this:
    <article class="new-post">
  <h2>New</h2>

  <article class="new-post__item">
    <h3>Hydrogen VS Electric Cars</h3>
    <p>Will hydrogen-fueled cars ever catch up to EVs?</p>
  </article>

  ...
</article>
    • You could replace the <div class="topic"> by an <article>.
    • The "Read More" looks like a <button> element but it is in fact an <a> element that will allow the user to navigate to a page where he/she will have access to the whole post. Therefore you should probably define a CSS rule that you can apply to an <a> element so that it looks like a button. πŸ˜‰
    • Instead of using CSS classes to handle responsive images, you could use the <picture> element together with the <source> element to achieve same like below:
    <picture>
  <source media="(min-width: 1110px)" srcSet="assets/images/image-web-3-desktop.jpg" />
  <source media="(min-width: 768px)" srcSet="assets/images/image-web-3-mobile.jpg" />
  <img src="assets/images/image-web-3-desktop.jpg" alt=alt="web3" />
</picture>

    CSS:

    • In general try to give meaning names to your CSS classes. This will not only help you as well as those you collaborate with you on a project to understand much faster what a particular CSS rule should be used for but will also help to organise your styles better.
    • Try to keep the level of nesting to a maximum of 3 levels deep as this hinders the reusability of your CSS rules. In other words, when you are overly specific, you will have to increase the nesting even more if you want to override any particular CSS rule. The rule of thumb is to define your generic rules first and then define more specific ones for other use cases.
    // DON'T β˜οΈπŸ‘Ž
main .content .top-content .top-right .text h3 {
  margin-bottom: 1rem;
  font-weight: 800;
}

// DO πŸ‘ŒπŸ‘
h3 {
  margin-bottom: 1rem;
  color: hsl(240, 100%, 5%);
  font-size: 1.25rem;
  font-weight: 800;
}

.article__item h3 {
  margin-bottom: 0.5rem;
  color: hsl(36, 100%, 99%);
  font-size: 1rem;
}

    RESPONSIVE:

    • On the tablet viewport, the content looks squeezed and there is a huge white space under the "The Bright Future of Web 3.0" section. It would look better if you set this top section in a single column layout on this viewport.

    I hope this helps.

    Keep it up.

    Marked as helpful
    0
  • Daveβ€’5,295
    @dwhensonSubmitted about 2 years ago

    This is my second project using React and first using styled components. I put some details in the readme, but some issues/questions I have are:

    • One issue I had with styled components is keeping track of whether the components in the file were now React components or styled components! I also struggled mapping the styled components to the rendered DOM in the dev tools, which made debugging a bit tricky in some cases. How to people manage this?
    • 'React.useEffect` is asking for additional dependencies in a couple of places in board.js (see FIXMEs). The only way I could get the app deployed was to ignore them. If I add them as suggested I end up with infinite loops. I wasn't sure how to deal with this if any one knows how to deal with this I would love to know.
    • Board.js is a bit of a monster, I had a look at refactoring things, but I couldn't see any obvious ways to do this. I considered moving the remaining game logic functions out, but they all change state so this didn't seem sensible. Any suggestions would be welcome!

    Thanks in advance for any suggestions and feedback. Cheers Dave

    Tic Tac Toe - React and Styled Components

    2
    Christopher Adolpheβ€’600
    @christopher-adolphePosted about 2 years ago

    Hi @Dave πŸ‘‹,

    You did a great job on this challenge. πŸ‘ I had a great time playing the game too. πŸ˜‰

    I saw you question about the issues you are having with dependency array of React's useEffect hook. I had a quick look at your code for the Board.js component and here are a few things that I have noticed which might be why you are facing those issues. I'll give you some rules/guidelines that I use myself when I work with the useEffect hook and I'll try to keep it simple so that you'll know where to go from here.

    Rule 1: Don't ignore the warnings about the dependency array

    • We get warnings when React sees we are doing something wrong. You should take a step back and carefully review your implementation of the useEffect hook.

    Rule 2: Know the data types of the dependencies being passed to the array

    • This is where most trouble come from. We should carefully check the data types of the different elements being passed to the dependency array. If the dependencies are primitive data types (i.e: string, number, boolean, null, undefined) we don't have much to worry as they are compared by value. But if they are reference data types (i.e array, object, function, date), the useEffect hook will compare them by reference (i.e their pointer in memory) and each time the component renders, we create a new reference to those dependencies and as the useEffect hook detects they are different, it reruns hence resulting in an infinite loop. 😱 Hopefully, React provides us with tools to work around this problem; they are the useMemo and useCallback hooks. Consider the useEffect below which was extracted from your Board.js component:
    React.useEffect(() => {
    if (gameType !== "CPU") return;
    if (marker !== turn) {
      renderSquareChoice(computerMove(squares, marker));
    }
  }, [gameType, turn]);

    renderSquareChoice is a function inside your component and you are using it inside useEffect, so React prompts you that it needs to be added to the dependency array. Now, you add it as a dependency, you get an infinite loop because renderSquareChoice is a function meaning it is a reference data type. To fix this, you need to wrap the renderSquareChoice with useCallback like so:

    const renderSquareChoice = useCallback((square) => {
    if (status || squares[square]) return;
    const nextSquares = [...squares];
    nextSquares[square] = turn;
    setSquares(nextSquares);
  }, []);

    And you will be able to add it to useEffect as a dependency without causing an infinite loop. NOTE: In the above code, useCallBack also has a dependency array, you might need to add status and squares as dependencies.

    • There are cases where we don't need to pass the entire object or array as a dependency to useEffect. For example, if my component has a person state/prop but my effect is only dependent on the age property of that person state. All we need to do is to pass this age property to useEffect like below:
    const [ person, setPerson ] = useState({
  name: 'Chris',
  age: 36,
  role: 'guest'
});

useEffect(() => {
  // some side effect that needs to run whenever the age of `person` state is mutated
}, [person.age]);

    So we are not passing the entire person state which an object and therefore a reference data type because each render of the component would create a new reference for the person state. Instead, we pass only the age property which is a number and therefore a primitive data type. I illustrated a simple case here but for complex cases the useMemo hook would be more appropriate. This feedback is already very long, I suggest you to read more about useMemo.

    Rule 3: Use the functional variant of the state setter function inside useEffect

    • Whenever useEffect is dependent on a state which is mutated inside it, you should use the functional variant of the state setter function. Taking another example extracted from the Board.js component:
    // Update total scores
  React.useEffect(() => {
    if (status === null) return;
    const newScore = { ...score };
    newScore[status] += 1;
    setScore(newScore);
  }, [status]);

    This useEffect should be refactored as follows:

    // Update total scores
React.useEffect(() => {
  if (status === null) return;
  
  setScore((prevScore) => {
    const newScore = { ...prevScore };

    newScore[status] += 1;

    return newScore;
  });
}, [status]);

    By doing so, our useEffect is no more dependent on score state and hence it isn't needed in the dependency array.

    I know that's a lot to read as feedback but I tried to keep it as lean as possible. useEffect is a big topic in itself. I also got these issues while tackling the Coffeeroasters challenge with React (still in progress πŸ˜‰). The more you practice, the better you'll get at it.

    I hope this helps you as a starting point.

    Here are some resources from Jack Herrington that helped me:

    Keep it up.

    Marked as helpful
    1
  • Christopher Adolpheβ€’600
    @christopher-adolphePosted over 2 years ago

    Hi @Briuwu πŸ‘‹

    You did a great job on this challenge. πŸ‘ Here are a few things that I have noticed and that you might want to check in order to improve your solution.

    • View plans and How we work are links that are styled as buttons. So your markup should be <a href="#">VIEW PLANS</a> instead of <button>VIEW PLANS</button>
    • In the navigation bar and the footer, How we work, Blog, etc these should be anchor tag as well.
    • There is a small issue in intro section on tablet. The image seems too large for this viewport width.
    • In the Footer.jsx component, you could also leverage on the map() method to generate the links by adding them to an array like you did in the Info.jsx component.

    src/data/footerLinks.js

    export const footerLinks = [
  {
    menuName: 'Our Company',
    items: [
      {
        title: 'How we work',
        link: '/how-we-work',
      },
      {
        title: 'Why insure ?',
        link: '/why-insure',
      },
      {
        title: 'View plans',
        link: '/view-plans',
      },
    ]
  },
  {
    menuName: 'Help me',
    items: [
      {
        title: 'FAQ',
        link: '/faq',
      },
      {
        title: 'Terms of use',
        link: '/terms-of-use',
      },
      {
        title: 'Policies',
        link: '/policies',
      },
    ]
  }
];

    src/components/Footer.jsx

    import { footerLinks } from '../data/footerLinks';
import { Link } from 'react-router-dom';

const Footer = () => {
  return (
    //...
    <div className="footer-container">
      {
        footerLinks.map((menu, index) => (
          <div className="footer-all" key={`menu-${index}`}>
            <h4 className='footer-title'>{ menu.title }</h4>
            {
              menu.items.map((item, index) => (
                <Link to="{item.link}" className="footer-info" key={`link-${index}`}>{item.title}</Link>
              )
            }
          </div>
        ))
      }
    </div>
    //...
  )
}

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Jason Moodyβ€’300
    @MoodyJWSubmitted over 2 years ago

    Hello, thanks for checking out my project!

    The worst part of it was updating Angular and killing all of the package vulnerabilities. I initially wanted to use Material as I'm familiar with it, but quickly realized it would be easier to just work from scratch than to modify Material components. I used a handful of RxJS operators as well.

    I'm happy to hear any suggestions, criticisms, advice, etc. I think my styling is pretty weak with regards to using the optimal approach; however, I think everything at least looks like it should on mobile/tablet/desktop.

    Responsive GitHub User Search Using Angular/TypeScript/RxJS/SCSS

    1
    Christopher Adolpheβ€’600
    @christopher-adolphePosted over 2 years ago

    Hi @MoodyJW πŸ‘‹

    You did a great job on this challenge. πŸ‘ The component composition is well done. πŸ‘Œ

    The only thing I have noticed is that the <body> tag is inside your root AppComponent and in the rendered index.html page you now have a <body> tag nested in another <body> tag which is unfortunately not valid. I understand you did this to implement the theme switching feature. Fortunately, you could resolve this with the help of Renderer2 from @angular/core module. By injecting this service and the DOCUMENT into your AppComponent class, you will be able to add/remove the theme class on the <body> element the "Angular way" without having it inside the root AppComponent template file. Your AppComponent should look something like this after doing this change:

    import { Component, OnInit, Inject, Renderer2 } from '@angular/core';
import {DOCUMENT} from "@angular/common";

/**
* Your other imports, component decorator etc
*/
export class AppComponent implements OnInit {
  theme = window.matchMedia('(prefers-color-scheme: light)').matches
    ? 'light-theme'
    : 'dark-theme';
  user!: User;

  constructor(
    @Inject(DOCUMENT) private document: Document,
    private renderer: Renderer2,
    private usersService: UsersService
  ) {}

  ngOnInit() {
    this.renderer.addClass(this.document.body, this.theme);
    this.usersService
      .getUser('octocat')
      .pipe(first())
      .subscribe((user) => {
          this.user = user;
    });
  }

  toggleTheme() {
    const bodyElem = this.document.body;
    const isLightTheme = bodyElem.classList.contains('light-theme');

    if (!isLightTheme) {
      this.renderer.removeClass(bodyElem, 'dark-theme');
      this.renderer.addClass(bodyElem, 'light-theme');
    } else {
      this.renderer.removeClass(bodyElem, 'light-theme');
      this.renderer.addClass(bodyElem, 'dark-theme');
    }
  }

  userReturned(user: any) {
    this.user = user;
  }
}

    However, with this change, the currentTheme argument passed to the toggleTheme() method would no more be required and therefore, the ThemeToggleComponent would no more need to emit a custom event. Even better, you could move the toggleTheme() method from the root AppComponent to the ThemeToggleComponent since it is the one responsible of this feature. The root AppComponent would thus be only responsible of initialising the user property.

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Alucardβ€’870
    @Alucard2169Submitted over 2 years ago

    I had fun building this project, i wanted to do this for a really long time, cause it looked cool πŸ˜‹.

    I hope you like it.

    feedback will be appreciated.

    Responsive design with HTML SCSS and JS

    1
    Christopher Adolpheβ€’600
    @christopher-adolphePosted over 2 years ago

    Hi @Alucard2169 πŸ‘‹

    You did a great job on the overall for this challenge. πŸ‘ I really like the subtle hover effect on the navigation links πŸ‘Œ. Here are a few things that I have noticed and that you might want to check in order to improve your solution.

    • It might be a good thing to have a skip to content link that redirects the user directly to main content is ever they are using assistive technology to visit this landing page.
    • In the vr section, the left and right paddings are not necessary for desktop as they offset the content while in the design, the content of this section is aligned with the next one.
    • In the creation section, I'm not sure if is a good option to use the images a background image as they seem to be part of the content rather than just decorative.
    • On tablet, the <header> is quite huge, maybe you should consider reducing the height and as for the main content, I think that there is ample space to display the creation section in a 2 column layout.

    I hope this helps.

    Keep it up.

    1
  • Christopher Adolpheβ€’600
    @christopher-adolphePosted over 2 years ago

    Hi @Reallyvane πŸ‘‹

    You did a great job in completing this challenge. πŸ‘ The component is faithful to the design. πŸ‘Œ Below are a few things that I have noticed and you might want to check in order to improve your solution.

    • When I look at this component, I would expect to see a <form> element with <input type="radio"/> elements for the different ratings and a submit <button> in the HTML markup. However, it is not the case in your solution. While the component seem to be working fine for the general users, those with assistive technology might struggle to interact with it. This is why I would suggest you to use HTML elements that are built to take user inputs when it is the case. For example <input type="radio"/> or <input type="checkbox"/> to choose from a list of options instead of <div> elements. This will also simplify things on the JavaScript side. πŸ˜‰
    • I also noticed that you have set a default rating value of 3 but at the moment, there is no visual cue that communicates that to the user. You should add the active class by default on the corresponding <div class="rating-btn"> element. Again, if it was a <input type="radio"/>, you would have used the checked attribute to achieve that. On the other hand, if you don't wish to set a default rating value, you would then have to add a check in the onSubmit() function like this:
    let stars_score;

function onSubmit() {
  // Returning early if stars_score is not set
  if (!stars_score) {
    return;
  }
  
  card_content_1.classList.add("hide");
  score.textContent = stars_score;
  card_content_2.classList.remove("hide");
}

    I hope this helps.

    Keep it up.

    Marked as helpful
    1
  • Tiffaniβ€’30
    @tiffanicodesSubmitted over 2 years ago

    I really struggled with figuring out the best grid units and the best number of columns to properly line up the gallery photos. I would love some input on this!

    Responsive Webpage using CSS Grid & Flexbox

    2
    Christopher Adolpheβ€’600
    @christopher-adolphePosted over 2 years ago

    Hi @tiffanicodes, πŸ‘‹

    You did a nice job in completing this challenge. πŸ‘ I have recently completed the same project and I think I can give you some tips on how you could improve your solution or at least think of a different approach.

    • I see that you have built the main content section using the grid-area property from CSS grid. While this is not a bad approach, it is not a flexible solution. Here are 2 resources that will surely help you come up with a more efficient layout. Travis Horn - Responsive grid, CSS-TRICKS - Responsive Grid
    • In order to render the hero title in black and white, you can apply the mix-blend-mode property. It can have different values depending on the result you want to get, in the case of this challenge, the exclusion value does the job. πŸ˜‰ Read more about it here.
    • The spacings in the footer section need to be reviewed as at the moment it looks wider than the header and main sections. I would suggest that you add a wrapping <div class="container"> element inside the different sections of your page to which you then apply a max-width. This will resolve the problem as well as on the location page.
    • On the CSS side, I would suggest that you refrain from using id as selectors to style elements as this hinders reusability.

    I hope this helps.

    Keep it up.

    Marked as helpful
    1