Skip to content
OverviewSolutions
3
Comments
7
fResult
@fResult

All comments

  • fResult•90
    @fResultPosted about 1 year ago

    LGTM solution krub.

    For my suggestion, it can be improved a bit by these...

    1. The Logic delete tag is not correct yet. In the App.jsx in the function handleDeleteTag()... for the selectTags.slice(0, -1), it will delete only the last tag, not the deleted tag. So, we can use filter instead. (Don't forget to get rid of if (!selectTags.includes(tag)) which wrapped the filtering logic)

      -    if (!selectTags.includes(tag)) {
-      const updatedTags = selectTags.slice(0, -1);
+      const updatedTags = selectTags.filter(selectedTag => selectedTag !== tag);
       setSelectedTags(updatedTags);

       const matchingItems = data.filter((item) => {
         return updatedTags.every((selectedTag) => {
           return (
             item.role === selectedTag ||
             item.languages.includes(selectedTag) ||
             item.tools.includes(selectedTag)
           );
         });
       });
       setFilteredItems(matchingItems);
-    }

      /components/SearchBar.jsx in the delete icon, send the word as an argument for the tag parameter in the handleDeleteTag function

      -                        onClick={handleClickDelete}
+                        onClick={() => handleClickDelete(word)}

      END 1

    2. state selectTags should be selectedTags. Bec of state should be noun while function should be verb.

      - const [selectTags, setSelectedTags = useState([])
+ const [selectedTags, setSelectedTags = useState([])

      END 2

    3. handleAddTag can be swap logic like this... from...

      const handleAddTag() {
  if (!selectTags.includes(tag)) {
    // handle logic
  }
}

      to... (return immediately if the tag is already existed in the selectedTags

      const handleAddTag() {
  if (selectTags.includes(tag)) return
  // handle logic
}

      END 3

    4. Reduce code for the checking every selectedTags logic

             const matchingItems = data.filter((item) => {
         return updatedTags.every((selectedTag) => {
-          return (
-            item.role === selectedTag ||
-            item.languages.includes(selectedTag) ||
-            item.tools.includes(selectedTag)
-          );
+          return [item.role, ...item.languages, ...item.tools].includes(selectedTag)
         });
        });

      OR we can use it like this (need knowledge about call, bind, apply a bit).

             const matchingItems = data.filter((item) => {
         return updatedTags.every([].includes.bind([item.role, ...item.languages, ...item.tools]));
       });

      END 4

    5. DRY (Don't Repeat Yourself) the duplicated logic in the handleAddTag and handleDeletetag

      5.1 Declare function to re-use in handleAddTag and handleDeleteTag.

        function filterBySelectedTags(selectedTags) {
    return function forItem(item) {
      return selectedTags.every((tag) => {
        return [item.role, ...item.languages, ...item.tools].includes(tag)
      })
    }
  }

      5.2 Use DRY function both in handleAddTag and handleDeleteTag

         const handleAddTag = (tag) => {
     if (selectTags.includes(tag)) return
     const updatedTags = [...selectTags, tag];
     setSelectedTags(updatedTags);

-    const matchingItems = data.filter((item) => {
-      return updatedTags.every((selectedTag) => {
-        return [item.role, ...item.languages, ...item.tools].includes(selectedTag)
-      });
-    });
+    const matchingItems = data.filter(filterBySelectedTags(updatedTags));
     setFilteredItems(matchingItems);
   };

    Moreover, you can learn more about useMemo and useCallback to make more performance for React Application krub. :)

    Marked as helpful
    0
  • fResult•90
    @fResultPosted almost 2 years ago

    For Country interface, array properties should use ...

    1. use the interface name by a single word such as Countries -> Country (let array ([]) say us it is a list)
    2. array or bracket ([]) after the interface name, see below snippet.
    export interface Country {
  name: string;
  nativeName: string;
  population: number;
  region: string;
  subRegion: string;
  capital: string;
  topLevelDomain: string[];
  currencies: Currency[];
  languages: Language[];
  borders?: string[];
  flags: Flags;
}

    Because the [Currency] means only one Currency object contains in the array. (It's not mean multiple Currency objects contains in an array). Sometimes we call [Currency] a tuple.

    Tuple reference: https://www.w3schools.com/typescript/typescript_tuples.php

    Marked as helpful
    0
  • fResult•90
    @fResultPosted almost 2 years ago

    Hello, your submission looks great. Especially, I like your Select/Options component. You styled the drop-down by yourself, and it made me amazing. :)

    And I have some suggestions to fix it.

    • I see the ERROR about Buttons must have discernible text. It could be fixed by aria-label="Show options". But I recommended don't use this icon as a button, just another tag (maybe <i> or <span>) is enough. (this icon in <Select> is not a landmark, just decorating for people who don't be disabilities.)

    • Fix ERROR <html> element must have a lang attribute by adding lang="en" to <html> tag

    Marked as helpful
    0
  • Prithivi Raj•140
    @prithiviraj275Submitted almost 2 years ago

    all feedbacks are welcome, thank you

    nft

    2
    fResult•90
    @fResultPosted almost 2 years ago

    Hello, @prithiviraj275.
    I have some suggestions to improve your submission. You could use this snippet to reset the style of margin and box-sizing.

    * {
   margin: 0;
   box-sizing: border-box;
}

    When you reset the margin to 0px, then <body>, <p>, <h1>, <h2>, ..., <h6>, and etc., will be gotten rid of default margin, then you can add margin or gap by urself.

    Next, the box-sizing: border-box; will change the box-sizing from content- box (default) to border-box which lets you easier to style padding without concern about the sum of sizing of padding and content in the box.

    References:

    Happy coding 😍

    0
  • fResult•90
    @fResultSubmitted almost 2 years ago

    New submission upgraded from the old one

    Stack:

    • Progressive Web Applicatoiin
    • Node 18 (upgraded from 12.0.0)
    • TypeScript 4.9.4 (upgraded from 4.0.3)
    • ReactJS 18.2.0 (upgraded from 17.0.1)
    • Axios 1.2.1 (upgraded from 0.21.1)
    • React Router 5.2.0
    • React Hook Form 6.14.0
    • Tailwind CSS 3.2.4 (upgraded from 1 (@tailwindcss/postcss7-compat))

    Plan to Upgrade: See more in my Gitlab...

    IF you have any suggestions, plz let me know. Thank you, thank you, thank you.

    TypeScript, Tailwind, ReactHookForm, PWA, A11Y without report issues

    1
    fResult•90
    @fResultPosted almost 2 years ago

    Updated:

    • Added Progressive Web Application (could install on Mobile)
    • Upgraded React version from 17.0.1 to 18.x
    • Upgraded Tailwind CSS version from 1 to 3.x
    • Upgrade Axios version from 0.21 to 1.x
    • Utilized proper type for react-hook-form's functions as prop
    • Fixed every error for Accessibility (A11Y)
    • Fixed Chevron down icon in Select component is not placed center

    Plan to Upgrade: See more in my Gitlab...

    0
  • fResult•90
    @fResultPosted almost 2 years ago

    Updated:

    • Added Progressive Web Application (could install on Mobile)
    • Upgraded React version from 16.9 to 18.x
    • Upgraded Tailwind CSS version from 1 to 3.x
    • Utilized proper type for react-hook-form's functions as prop
    • Fixed every error for Accessibility (A11Y)
    • Fixed Chevron down icon in Select component is not placed center

    Plan to Upgrade: See more in my Gitlab...

    0
  • fResult•90
    @fResultPosted almost 3 years ago

    Updated:

    • Upgraded REST Countries API version from v1 to v2.
    • Refactor some logic and some type which they are any.
    0