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

Feat: create header #1

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Feat: create header #1

wants to merge 1 commit into from

Conversation

Godbell
Copy link

@Godbell Godbell commented Jul 21, 2024

requires env var VITE_API_URL for api base url and VITE_SESSION_SECRET for cookie session secret

@Godbell Godbell requested a review from aube-dev July 21, 2024 13:06

const tokenKey = 'token';

export const getUser = async (): Promise<User | null> => {
Copy link
Member

Choose a reason for hiding this comment

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

서버에서만 실행되어야 하는 것 같은데, 실수로 클라이언트에서 사용하지 않도록 장치가 있으면 좋을 듯합니다!
참고: https://remix.run/docs/en/main/discussion/server-vs-client

Comment on lines +1 to +7
export enum AuthError {
UNAUTHORIZED,
EMAIL_BLANK,
EMAIL_FORMAT_INVALID,
EMAIL_OR_PW_INVALID,
UNEXPECTED,
}
Copy link
Member

Choose a reason for hiding this comment

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

사소한 거긴 한데, enum은 tree shaking에서 약간 불리하다는 이야기가 있어 이렇게 고치면 어떨까요?

export const AuthError = {
  UNAUTHORIZED: 'UNAUTHORIZED',
  EMAIL_BLANK: 'EMAIL_BLANK',
  EMAIL_FORMAT_INVALID: 'EMAIL_FORMAT_INVALID',
  EMAIL_OR_PW_INVALID: 'EMAIL_OR_PW_INVALID',
  UNEXPECTED: 'UNEXPECTED',
} as const;
export type AuthError = typeof AuthError[keyof typeof AuthError];

참고: https://engineering.linecorp.com/ko/blog/typescript-enum-tree-shaking

body?: Object;
}

export const sendRequest = async (args: requestArgs) => {
Copy link
Member

Choose a reason for hiding this comment

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

sendRequest는 'request'라는 말이 지칭하는 게 뭔지 너무 광범위하다 보니 약간 모호한 것 같아요.
fetchApi 같이 fetch를 사용한다는 걸 명시하거나 requestApi 같이 어떤 걸 요청하는지 한정하는 등의 네이밍은 어떨까요?

@@ -0,0 +1,12 @@
interface requestArgs {
Copy link
Member

Choose a reason for hiding this comment

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

다른 곳에서 Props 같은 타입의 경우에 PascalCase로 네이밍하고 있어서 통일하면 좋을 듯해요!

interface requestArgs {
apiPath: string;
method?: 'GET' | 'POST' | 'DELETE' | 'PUT';
body?: Object;
Copy link
Member

Choose a reason for hiding this comment

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

ObjectArray, Function 타입 등도 허용해 버리는데 혹시 의도한 부분일까요? { key: 'value' } 같은 객체만 허용하는 의도라면 Record{ [key: string]: string | number } 등 다른 방법을 사용해 보면 어떨지 제안합니다!

export const AppHeaderLink = (props: Props) => {
return (
<NavLink
className={({ isActive, isPending }) => {
Copy link
Member

Choose a reason for hiding this comment

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

isPending은 사용하지 않는 것 같아서 따로 destructuring하지 않아도 될 듯합니다!

Comment on lines +18 to +38
{user === null ? (
<>
<li className="nav-item">
<AppHeaderLink caption="Home" linkTo="/"></AppHeaderLink>
</li>
<li className="nav-item">
<AppHeaderLink
caption="Sign in"
linkTo="/login"
></AppHeaderLink>
</li>
<li className="nav-item">
<AppHeaderLink
caption="Sign up"
linkTo="/register"
></AppHeaderLink>
</li>
</>
) : (
<></>
)}
Copy link
Member

@aube-dev aube-dev Jul 21, 2024

Choose a reason for hiding this comment

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

이렇게 따로 관리하면 .map으로 일관되게 렌더링하니 각 메뉴마다 혹시 다른 스타일을 적용할 수도 있는 실수를 예방할 수 있고 좀 더 깔끔할 듯해서 제안해 봅니다!

const guestNavItems = [
  {
    label: 'Home',
    link: '/',
  },
  {
    label: 'Sign in',
    link: '/login',
  },
  {
    label: 'Sign up',
    link: '/register',
  },
] as const;

const userNavItems = [] as const;

export const AppHeader = () => {
  const user = useAuth();

  return (
    <nav className="navbar navbar-light">
      <div className="container">
        <Link className="navbar-brand ng-binding" to"/">
          conduit
        </Link>
        <ul
          className="nav navbar-nav pull-xs-right"
          style={{ display: 'inherit' }}
        >
          {(user === null ? guestNavItems : userNavItems).map((navItem) => (
            <li key={navItem.label} className="nav-item">
              <AppHeaderLink caption={navItem.label} linkTo={navItem.link} />
            </li>
          ))}
        </ul>
      </div>
    </nav>
  );
};

@@ -0,0 +1,12 @@
interface requestArgs {
apiPath: string;
method?: 'GET' | 'POST' | 'DELETE' | 'PUT';
Copy link
Member

Choose a reason for hiding this comment

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

methodstring이 아니라 직접 literal로 제한하는 거 좋은 것 같아요 👍
PATCH도 추가하면 좋을 듯합니다!

@@ -0,0 +1,12 @@
interface requestArgs {
apiPath: string;
Copy link
Member

Choose a reason for hiding this comment

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

사용 사례를 보니 '/'로 항상 시작해야 하는 것 같은데 맞을까요? 맞다면 조금 더 strict하게 이렇게 정의하는 것도 제안해 봅니다!

Suggested change
apiPath: string;
apiPath: `/${string}`;

Comment on lines +2 to +3
import { AppHeaderLink } from './AppHeaderLink';
import { useAuth } from 'hooks/useAuth';
Copy link
Member

Choose a reason for hiding this comment

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

다른 곳을 보니 절대경로를 사용하고 있어서 절대경로로 통일하면 어떨까요? 나중에 이 파일(app/components/AppHeader.tsx)의 디렉토리가 바뀌더라도 이 파일의 import문들은 수정할 필요가 없어져서 편할 듯합니다!

Suggested change
import { AppHeaderLink } from './AppHeaderLink';
import { useAuth } from 'hooks/useAuth';
import { AppHeaderLink } from '~/components/AppHeaderLink';
import { useAuth } from '~/hooks/useAuth';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants