lada496 / star-wars-list Goto Github PK

          It's better for you to abstract this function at a higher level as you have assigned `BrowserRouter` in multiple places too. Prob you can even combine this with Redux's `<Provider />` as well so that you won't need to invoke different functions on each test cases

Originally posted by @kei95 in #29 (comment)

You may start a new if statement instead of using `else if` statement

          You may start a new if statement instead of using `else if` statement because once the first if statement meets the condition, it returns and stops the flow.

Originally posted by @koh1project in #34 (comment)

name this variable something that tells that it is not real data

    [nitpick] - you wanna name this variable something that tells that it is not real data. I would name it something like the followings;

mockedModifiedCharacters
modifiedCharactersFixture

Originally posted by @kei95 in #10 (comment)

change detailed character view to be a modal

put lines b/w code snippet

          I found some codes are too to each other that it's harming readability a bit. For these lines case, prob you can do like;

        const newRenderedCharacters = [...state.renderedCharacters]
        sort(newRenderedCharacters, action.sortFactor)
        
        return {
          ...state,
          renderedCharacters: newRenderedCharacters,
          sortFactor: action.sortFactor,
        }
      }
      
      const homelandTarget = state.filterTargets.homelandTarget
      const genderTarget = state.filterTargets.genderTarget
      let newRenderedCharacters = state.originalCharacters

Originally posted by @kei95 in #34 (comment)

It feels like this file's component is `page` rather reusable `layout`?

    It feels like this file's component is `page` rather reusable `layout`? I'd expect layout to contain `children` or some sort of logic to render subsequent components without any logic. Means layout typically is dumb component

Originally posted by @kei95 in #2 (comment)

provide geek mode to show sort and filters

this has to be applied after #48 implementation

change sort factors

alphabetical order
homeland alphabetical order

re-think folder structure where to put redux related types

    using a name "index" for this file is a bit too vague. You are not using an index export pattern and this file contains types for API response shape so I'd say it makes more sense to be under `/api`. Be careful of using `index-` for file name as it easily contaminates your project coz they are all the same name so the more you name files `index-` the less searchability your project has.

Originally posted by @kei95 in #2 (comment)

change showDetails to isShowDetails

to explicitly show this is a boolean variable
https://www.notion.so/Story1-User-can-fetch-Star-Wars-Data-a4c97c7b0e634c61844e8d7a071def16

delete unnecessary comment

          You must've tired 😄

Originally posted by @kei95 in #34 (comment)

have right gray border for character images

It could be something like; `sortKey` or `sortFactor`

          [nitpick] - I'm not really impressed by this type's name as `Key` is way too vague. It could be something like; `sortKey` or `sortFactor` or something else.

Originally posted by @kei95 in #23 (comment)

set custom color theme

Previous design:
Navigation and other components use different blacks

After fix:
Use the same black:

instead of gender use era for the second filter

make a custom hook by extracting the logic

    > I'd love to receive a feedback in AllSpecies.tsx. Especially I want to know where is the good place to put reducer functions.

I wouldn't mind having this reducer logic in this file as it's small enough and encapsulated in this domain. Though it's ok to leave them here, if you don't feel comfortable leaving them in this file then I'd advise you to make a custom hook by extracting the logic so that you can separate the logic part of this file and render part of it. It could be called something like; useSortCharacters

Originally posted by @kei95 in #23 (comment)

I prefer to generate a new array before execute `sort()`. I notice you understand that `sort()` is destructive and you pass a new array in some conditions. But, you sometimes assign a reference to a variable, for example, line 159 `let newRenderedCharacters = state.originalCharacters`. So far you can manage them but it may be confusing if another dev needs to modify.

          I prefer to generate a new array before execute `sort()`. I notice you understand that `sort()` is destructive and you pass a new array in some conditions. But, you sometimes assign a reference to a variable, for example, line 159 `let newRenderedCharacters = state.originalCharacters`. So far you can manage them but it may be confusing if another dev needs to modify.

Originally posted by @koh1project in #34 (comment)

I'd name this folder something like `testHelpers`

    I'd name this folder something like `testHelpers` coz `mock` refers to fake static values rather than the logic to generate ones. It's a bit subjective tho.

Originally posted by @kei95 in #10 (comment)

declare this value with `as const`

    [nitpick] - to tell other devs that it won't change or not meant to be mutated, you wanna declare this value with `as const` as it "locks" the value from TS perspective and make each object's values to be literal types rather than primitive ones(e.g. mass: number -> mass: 81 )

https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#literal-inference

Originally posted by @kei95 in #10 (comment)

move the functions outside of the component

          II feel you can make a hooks file and move the functions in this component file into it because this component now contains several function declarations and may become a mess.

Originally posted by @koh1project in #34 (comment)

view change button on Navbar and provide a list view

create a clear filter button

setting up a github action

    @Lada496 Now that you have quite a decent testing coverage for your project, do you wanna try setting up a github action to run unit/integration tests upon each push on all branches? It's important to automate tests so that you won't need to run them locally and that results in preventing your teammates to be pushing untested broken code by accident since the automation runs and tells its result no matter what.

Originally posted by @kei95 in #10 (comment)

I'd name this folder something like `testHelpers` coz `mock` refers to fake static values rather than the logic to generate ones. It's a bit subjective tho.

    I'd name this folder something like `testHelpers` coz `mock` refers to fake static values rather than the logic to generate ones. It's a bit subjective tho.

Originally posted by @kei95 in #10 (comment)

change filter and sort logic

always re-create characters list with current sortTarget, homelandTarget, genderTarget

use Pick<> for `ModifiedCharacter`

star-wars-list/src/api/starWars-types.ts

I'd use ternary operator instead

    Double ampersands(`&&`) operator is considered to be a bad practice in JSX due to its behavior. It acts undesired when the evaluated value is not `boolean` like when the value is `undefined`. I'd use ternary operator instead(value ? option1 : option2)

reference: https://medium.com/geekculture/stop-using-for-conditional-rendering-in-react-a0f7b96200f8

Originally posted by @kei95 in #10 (comment)

have search bar

necessary to consider to change entire app structure

you could've done it with `map`?

    [nitpick] - porb you could've done it with `map`? Not a big deal though.

Originally posted by @kei95 in #2 (comment)

preventable to get an "empty" character with an id

    I see how you killed the bug, but it seems somewhat hacky. Isn't it preventable to get an "empty" character with an id in the first place somehow?

Originally posted by @kei95 in #23 (comment)

re-think folder structure where to put api related functions

    This file can be under an independent `api` folder rather than util since you will have more logics that are tightly coupled with an API request/response treatment. `utils` get big and messy so try to avoid using this folder as much as you can by coupling some files under a single folder.

The following link is for an article that is about file structure for react project. Ngl I doesn't fully agree with it(especially its usage of test and util) but it generally makes sense. You might wanna casually check on API folder to see what it would look like.

https://dev.to/larswaechter/how-i-structure-my-react-projects-jii

Originally posted by @kei95 in #2 (comment)

create error card component

          Is this TODO comment still relevant? 👀 If so, might be better to mention when to fix as TODO comments can be abandoned quite quickly. The most common way to do so is to mention the ticket that fixes it. If you don't have one, create it so that it's forcing you to at least have some ticket to work on in the future. It's better for other developers who don't have context too.

Originally posted by @kei95 in #34 (comment)

`View details` button remains even the hidden items were shown

change this to show "Hide details" when showing character details

    Ah ic I thought it was a bug that `View details` button remains even the hidden items were shown

Originally posted by @kei95 in #10 (comment)

You might wanna add a test that gives mocked response from the API consumed in App and check if expected elements are shown.

    You might wanna add a test that gives mocked response from the API consumed in App and check if expected elements are shown. And make sure to add tests for each component too as you might be able to cache side effects in other changes. Even snapshots can be sufficient for them.

Originally posted by @kei95 in #2 (comment)

put this under API folder

    Might be better to put this under API folder as it's the closest place that the original value(the type definition and the logic to retrieve this data in this case).

Originally posted by @kei95 in #10 (comment)

set tile file under the same level of original implementation

    I'd personally prefer to set tile file under the same level of original implementation such as below;

// no need for tests folder
CharacterDetails.tsx
CharacterDetails.test.tsx
CharacterItem.tsx
CharacterItem.test.tsx
[...]

And the mock data can be close to where its original value is defined. In this case, you can declare modifiedCaracter(mocks/modifiedCharacters.tsx) under API folders as that's where you define it.

Originally posted by @kei95 in #10 (comment)

fix the file structure

I found that the file structure is a bit odd. Currently, pretty much all the UI files are under components/ but there're multiple files that are mentally bigger than "components" such as AllSpecies.tsx. For such a file, you might want to have page folder and you can put AllSpecies.tsx and SpeciesItem.tsx. Also, UI folder can be de-structured and put on the same level as other components. Something like the following image is what I mean, but we can have a quick call to go over this structure once you are back from Japan 🙂

Originally posted by @kei95 in #53 (comment)

narrow this type to a union of string literals

    I think you can narrow this type to a union of string literals. In this use case, you know the selected value is always either `height` or `mass`, so I'd recommend you create an enumerator-like object so that you can couple all the logic looking at the single source of truth. This is phenomenally effective when all the logic is based on a single factor/domain. In this case, we know they all are trying to "sort" characters with these keys, so it's ok to couple them together to look at the single place.

This is just a pseudo code so don't count on it lol but I hope you get the idea.

// enum like object - make sure to leave a comment about why you use this object as it has multiple dependencies. 
// Means that a single change on this object can cause a big mess 
const SORT_FACTOR = {
  MASS: "mass",
  HEIGHT: "height",
  DEFAULT: "default" // you might not need this value
} as cosnt

// "mass" | "height" | "default"
type SortFactor = typeof SORT_FACTOR[keyof typeof SORT_FACTOR]

type CharacterState = {
  originalCharacters: ModifiedCharacter[]
  modifiedCharacters: ModifiedCharacter[]
  selectedValue: SortFactor // here, you narrowed the type from "string" to "mass" | "height" | "default"
}

// this function now can accept "mass" | "height" | "default". You might want to handle the case "default" explicitly
const sort = (characters: ModifiedCharacter[], key: SortFactor) =>
  characters.sort((a, b) => (b[key] || 0) - (a[key] || 0))

[...]

{
    Object.keys(SORT_FACTOR).map(sortFactorKey => (
        // You might need some logic or CSS trick to change the value to the title case
        <option value={SORT_FACTOR[sortFactorKey]}>{SORT_FACTOR[sortFactorKey]}</option>
     ))
}

Originally posted by @kei95 in #23 (comment)

create loading component

          Is this TODO comment still relevant? 👀 If so, might be better to mention when to fix as TODO comments can be abandoned quite quickly. The most common way to do so is to mention the ticket that fixes it. If you don't have one, create it so that it's forcing you to at least have some ticket to work on in the future. It's better for other developers who don't have context too.

Originally posted by @kei95 in #34 (comment)

I'd put this component under `components/`

    I'd put this component under `components/` as it doesn't serve as a "layout" since it's just a dumb component that renders footer

Originally posted by @kei95 in #8 (comment)

you could use `queryByText` once and remove `height` variable in line 46

    [nitpick] - you could use `queryByText` once and remove `height` variable in line 46

Originally posted by @kei95 in #10 (comment)

name more explicitly

          [nitpick] - It might be better to rename this variable, as `value` is a bit too generic. Other devs might think; _"What kind of value do you mean?"_

Originally posted by @kei95 in #34 (comment)

lada496 / star-wars-list Goto Github PK

star-wars-list's Introduction

Star Wars List

Description

Purpose

Stack

star-wars-list's People

Contributors

Watchers

star-wars-list's Issues

Recommend Projects

Recommend Topics

Recommend Org