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

DataTableを初期Sort設定をPropで受け取れるようにする #80

Merged
merged 9 commits into from
Jun 19, 2020

Conversation

maktak1995
Copy link
Contributor

close #32

  • Propsに defaultSortFielddefaultSortOrder を追加
  • 関連する処理を追加

@maktak1995 maktak1995 requested a review from youchann June 16, 2020 08:27
@youchann
Copy link
Contributor

export type Column<T, C> = {
  name: C;
  selector: (data: T) => string | number;
  sortable?: boolean;
  width?: string;
  renderCell?: (data: T) => React.ReactNode;
  align?: TypographyProps["align"];
};

type Tab<T> = {
  label: string;
  filter: (data: T[]) => T[];
  disabledCheck?: boolean;
};

type Props<T, C> = {
  data: T[];
  columns: Column<T, C>[];
  enablePagination?: boolean;
  onSelectRowsChange?: (rows: number[]) => void;
  onRadioChange?: (radio: number) => void;
  clearSelectedRows?: boolean;
  tabs?: Tab<T>[];
  tabWidth?: string; // tabsがある時のみ有効
  emptyTitle?: string;
  emptySubtitle?: string;
  per?: number; // perが指定されている場合、初期値がそれに強制されます
  defaultSortField?: C;
  defaultSortOrder?: "desc" | "asc";
};

// idを必須にしたい
const DataTable = <
  T extends { id: number; selectDisabled?: boolean },
  C extends string
>({
  data: sourceData,
  columns,
  enablePagination = false,
  onSelectRowsChange,
  onRadioChange,
  clearSelectedRows,
  tabs,
  tabWidth,
  emptyTitle,
  emptySubtitle,
  per,
  defaultSortField,
  defaultSortOrder,
}: Props<T, C>) => {

これで型がいい感じになったんだけれど、もっと良い方法募集中:thinking:

Copy link
Contributor

@youchann youchann left a comment

Choose a reason for hiding this comment

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

ちょっと難易度高いのですが、ぜひやってみてほしいです!!

);

// 初回表示時にdefaultSortFieldがなければ一番左側のsortableなcolumnを基準にソートする
const onSetFirstSortableColumn = (defaultSortField?: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

一箇所でしか使われていないので、変数化しなくても良さそうですね!!

あるいはコンポーネント外で関数化するか。。

Copy link
Contributor

Choose a reason for hiding this comment

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

こんな感じの方がスリムですね

  const onSetFirstSortableColumn = (defaultSortField?: string) => {
    const selectedColumn = columns.find(
      (column) => column.name === defaultSortField,
    );
    return selectedColumn?.sortable
      ? selectedColumn
      : columns.find((column) => column.sortable === true);
  };

Copy link
Contributor

Choose a reason for hiding this comment

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

#80 (comment)
↑みたいに型を絞る(欲を言えばsortableなcolumnのnameだけを入れれるようにする)みたいなやり方ができないか、少し調べてみてほしいです!!

※無理だったらstring型のままで、

こんな感じの方がスリムですね

の実装でOKです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

やってみます!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

いろいろ調べてみましたがいい感じの方法が見つからなかったので、

こんな感じの方がスリムですね

の実装のみとさせてください mm

Copy link
Contributor

Choose a reason for hiding this comment

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

全然大丈夫です!:+1:

Comment on lines 18 to 24
order?: "desc" | "asc",
) =>
React.useState<CurrentSortState<T>>({
getValue,
name,
isDesc: order === "desc",
});
Copy link
Contributor

Choose a reason for hiding this comment

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

この辺思ったのですが、

export const useOrderState = <T>(defaultState: CurrentSortState<T>) =>
  React.useState<CurrentSortState<T>>(defaultState);

↑の方がわかりやすい気がしません?(相談)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yutaro1031
たしかにsort側のコードはシンプルになってわかりやすくなりますが、こうするとDataTableで利用するときに

  const [sortState, setSortState] = useOrderState<T>({
    isDesc: defaultSortOrder === "desc",
    name: firstSortableColumn ? firstSortableColumn.name : "",
    getValue: firstSortableColumn ? firstSortableColumn.selector : undefined,
  });

という感じに引数をオブジェクトにまとめてから渡すことになりますよね。

この形であればそれぞれの引数の意味がわかるので可読性はあがるのはいいと思うんですけど、useOrderState を使う側が CurrentSortState の形式をわかっていないといけなくなりますよね。
importしてDataTableのなかで型定義に使ってますし、そこは気にしなくてもいいでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こっちのやりかたでいきますね

@youchann youchann self-assigned this Jun 17, 2020
@@ -125,6 +127,8 @@ const DataTable = <T extends { id: number; selectDisabled?: boolean }>({
emptyTitle,
emptySubtitle,
per,
defaultSortField,
defaultSortOrder,
Copy link
Contributor

Choose a reason for hiding this comment

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

今だと初期値がuseOrderStateに依存してるので、

Suggested change
defaultSortOrder,
defaultSortOrder = "desc",

とした方が読みやすそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

たしかにそうですね!修正します

@maktak1995 maktak1995 requested a review from youchann June 19, 2020 05:59
Copy link
Contributor

@youchann youchann left a comment

Choose a reason for hiding this comment

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

LGTMです!!

コメした部分の変更は任せます。(どちらでも変わらないはず)

src/components/DataTable/DataTable.tsx Outdated Show resolved Hide resolved
Co-authored-by: 林祐太郎 <40020812+yutaro1031@users.noreply.github.com>
@maktak1995 maktak1995 merged commit bfc6849 into master Jun 19, 2020
@maktak1995 maktak1995 deleted the issue-32-able-to-set-first-sortable-column branch June 19, 2020 07:01
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.

DataTable: firstSortableColumnをpropsで渡せるようにする
3 participants