Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/main page API 연동 완료 #16

Merged
merged 11 commits into from
Sep 6, 2024
Merged

Conversation

2NNS-V
Copy link
Collaborator

@2NNS-V 2NNS-V commented Sep 5, 2024

구현한 기능

  • 통계 페이지, 메인 통계 연동

논의하고 싶은 내용

기타

  • 통계페이지 UI 수정중입니다

@2NNS-V 2NNS-V added the enhancement New feature or request label Sep 5, 2024
@2NNS-V 2NNS-V self-assigned this Sep 5, 2024
Comment on lines 26 to 43
const {
data: allData,
loading,
error,
} = useAxios<axiosProps>(
{
url: `/stats/top/mbti`,
method: "GET",
},
[],
);

const { data: majorData } = useAxios<axiosProps>(
{
url: `/stats/top/department?key=${selectedMajor}`,
method: "GET",
},
[selectedMajor],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useAxios 커스텀 훅을 사용해서 추상화 하셨군요! 좋습니다
저는 NodeJS 백엔드도 같이 공부를 하다 보니 API 요청에 대해 한번더 추상화를 진행하는 편입니다

  1. api 라는 axios 인스턴스를 만들고,
    https://github.com/Principes-Artis-Mechanicae/get-p-frontend/blob/develop/src/config/axios.ts
  1. 이런식으로 도메인마다 서비스 레이어를 만들어서 서비스레이어에서 예외처리와 데이터 전처리를 담당하도록하고,
    https://github.com/Principes-Artis-Mechanicae/get-p-frontend/blob/develop/src/services/member/service.ts
  1. 페이지단이나, Redux Thunk Action, Tanstack Query 의 queryFn 에서 해당 서비스를 호출하는 식으로 작성하는 편입니다
    https://github.com/Principes-Artis-Mechanicae/get-p-frontend/blob/develop/src/store/thunk/auth.thunk.ts

제가 짠 코드가 정답은 아니니 이런방식도 있구나 하시고
괜찮으면 이렇게 레이어를 나눠서 작성하시는 것도 좋아 보입니다!


export default function AnalyticsPage() {
const [selectedMajor, setSelectedMajor] = React.useState<string>("");
const [selectedMajor, setSelectedMajor] = React.useState<string>("humanities");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectedMajor 에 인문대학에 해당하는 값을 기본적으로 넣는것 보다, null 과 같은 값을 넣어두고,
department 를 선택하지 않은 경우, 에러 메시지를 띄우는 것이 좋아보입니다!

그리고 상태에 해당하는 값또한 string 보다는 enum 타입으로 따로 분리하면 좋을 것 같아요

Copy link
Collaborator Author

@2NNS-V 2NNS-V Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 enum 타입을 사용하는 이유가 있을까요? API를 보낼 때마다, setDepartment() 마다 DropDown의 value(string)을 아래처럼 선언된 Department 타입으로 변경해야 하는데, enum을 사용할 필요가 있을까요? 아니면 서버에서 받기 쉬워서 그런건가요?

export enum Department {
    Humanities = "humanities",
    SocialSciences = "social-sciences",
    NaturalSciences = "natural-sciences",
    Economics = "economics",
    Engineering = "engineering",
    IT = "it",
    Agriculture = "agriculture",
    Arts = "arts",
    Teachers = "teachers",
    Medicine = "medicine",
    Dentistry = "dentisty",
    Vet = "vet",
    HumanSciences = "human-sciences",
    Nursing = "nursing",
    Pharmacy = "pharmacy",
    AdvancedTechnology = "advanced-technology",
    Environment = "environment",
    ScienceTechnology = "science-technology",
    Administration = "administration",
    Undeclared = "undeclared",
    default = "",
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 서버에서 받기 쉬운것보다 enum 타입으로 선언하면
Department.Humanities 로 IDE 나 코드 편집기에서 자동완성을 해주기 때문에,,, 자잘한 오류를 방지하기 위해서 주로 사용합니다!
여러 군데에서 해당 department 의 값을 사용할때 사용하면 좋을것 같아요~

Copy link
Member

@toothlessdev toothlessdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@toothlessdev toothlessdev merged commit 4713226 into develop Sep 6, 2024
1 of 2 checks passed
@toothlessdev toothlessdev deleted the feature/main-page branch September 6, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants