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

Table系のリニューアル #126

Merged
merged 20 commits into from
Jul 20, 2020
Merged

Table系のリニューアル #126

merged 20 commits into from
Jul 20, 2020

Conversation

youchann
Copy link
Contributor

@youchann youchann self-assigned this Jul 15, 2020
@youchann
Copy link
Contributor Author

memo

  • fullWidthか否かで、両サイドのborderおよびborder-radiusの出し分けをした
  • 一番外側のborderを2pxにした
    • overflow: hiddenを入れると<th />をstickyにできない
    • 1pxだと中の要素がはみ出るので、2pxにしている状態
  • 縦線アリのバージョンが必要か要確認

@youchann youchann changed the title WIP: Table系のリニューアル Table系のリニューアル Jul 16, 2020
Comment on lines 30 to 31
table-layout: auto;
white-space: nowrap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

デフォルトだとtable-layout: fixed;white-space: normal;だと思うのですが、理由があったりしますかね....?
特にwhite-spaceの方は改行させるとき、使う側が明示的に<br />を入れたりすると思うので、nowrapでも良いのでは??と思っています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

あーでもDSTのデータは長い文字列があるのか。。。

Copy link
Contributor

Choose a reason for hiding this comment

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

枠名とかがめっちゃ長いんよねw
table-layout: fixed;に関してはそうしないとwidthが聞かないはず

scrollableにする時は table-layout: auto;でいいかな

なのでDataTable的に横をscrollableにするかどうかを選べると良さそう

Comment on lines +117 to +119
verticalSpacing?: VerticalSpacing;
fullWidth?: boolean;
tableMaxHeight?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • verticalSpacing → cellの余白を3種類で定義
  • fullWidth → top以外のborderおよびborder-radiusを出すか否かを制御
  • tableMaxHeight → 文字通りテーブルのmaxHeightを定義。<th />はデフォでsticky

Comment on lines +310 to +322
<Styled.BorderContainer fullWidth={fullWidth}>
{!!tabs && (
<TableTabs
width={tabWidth}
value={currentTabIndex}
items={tabs.map((tab, index) => ({
label: tab.label,
value: index,
}))}
onChange={onHandleTabChange}
/>
)}
<Styled.TableContainer maxHeight={tableMaxHeight}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

変更量多く見えるけど

�- Styled.BorderContainer

  • Styled.TableContainer

が加わっただけ。

Comment on lines 20 to 21
/* TODO: 他に透明度指定する方法を聞く */
box-shadow: 0 4px ${colors.basic[300]}3D;
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

Choose a reason for hiding this comment

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

hexToRgbaみたいな関数作ったのでそれ使ってもらえると!

Comment on lines 14 to 17
& th {
position: sticky;
top: 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

デフォでsticky

Copy link
Contributor

Choose a reason for hiding this comment

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

意図しないとこでstickになったりしないかな

Copy link
Contributor

Choose a reason for hiding this comment

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

optionで渡す方が安全なきがする

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tableMaxHeightpropsを渡した時にstickyにするようにします!

Comment on lines +18 to +33
& > thead > tr:first-of-type > th {
&:first-of-type {
border-top-left-radius: ${Radius.SMALL};
}
&:last-of-type {
border-top-right-radius: ${Radius.SMALL};
}
}
& > tbody > tr:last-of-type > td {
&:first-of-type {
border-bottom-left-radius: ${Radius.SMALL};
}
&:last-of-type {
border-bottom-right-radius: ${Radius.SMALL};
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

角がはみ出ないようにするためのプロパティ。

padding: ${({ theme }) => theme.spacing}px
${({ theme }) => theme.spacing * 2}px;
/* TODO: 他に透明度指定する方法を聞く */
box-shadow: 0 4px ${colors.basic[300]}3D;
Copy link
Contributor

Choose a reason for hiding this comment

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

colors使わなくていいようにしたいな...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

適したpaletteがなくて、直接入れようという話をnoroさんとしたんですよねぇ...

paletteにプロパティを追加するor既存のプロパティを変えることができれば良いのですが、妙案あればぜひに。

Copy link
Contributor

Choose a reason for hiding this comment

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

colors.basic[300] が特に固定で書く回数多い気がするからどこかに入れれるといいね

Copy link
Contributor Author

Choose a reason for hiding this comment

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

一応issued
#129

Copy link
Contributor

Choose a reason for hiding this comment

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

ありがと!

{children}
</Typography>
{sortable && (
<Styled.IconContainer>
<>
<Spacer pl={0.5} />
Copy link
Contributor

Choose a reason for hiding this comment

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

FragmentじゃなくてSpacerのchildrenに入れるのが良さそう

export type VerticalSpacing = "small" | "medium" | "large";

const verticalSpacingMap: { [key in VerticalSpacing]: string } = {
small: "8px",
Copy link
Contributor

Choose a reason for hiding this comment

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

themeのspacingを使って定義して欲しい!

Comment on lines 14 to 17
& th {
position: sticky;
top: 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

意図しないとこでstickになったりしないかな

Comment on lines 14 to 17
& th {
position: sticky;
top: 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

optionで渡す方が安全なきがする

@@ -292,30 +321,6 @@ export const CustomCell: React.FunctionComponent = () => {
);
};

export const WithStickyHeader = () => (
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 Author

Choose a reason for hiding this comment

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

デフォでstickyにしたからですね。

#126 (comment) をやるにあたってまた追加すると思います。

@@ -322,7 +322,7 @@ const DataTable = <T extends { id: number; selectDisabled?: boolean }>({
<Styled.TableContainer maxHeight={tableMaxHeight}>
Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど
noneが初期値なのか

& > th {
${({ isStickyHeader }) =>
isStickyHeader
? `
Copy link
Contributor

Choose a reason for hiding this comment

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

styled-componentsのcssを使って
isStickyHeader && css`` 的な感じで書くと良さそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そういえばそんなAPIありましたね。。
ちょっとみておきますー

@@ -6,11 +6,18 @@ import { Header } from "./Header";
import { Row } from "./Row";
import { Radius } from "../../../../styles";

const Container = styled.table`
type TableProps = {
horizontalScrollable: boolean;
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

@kinokoruumu kinokoruumu left a comment

Choose a reason for hiding this comment

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

cssのとこ以外はLGMTです!

@youchann youchann merged commit a859f4d into master Jul 20, 2020
@youchann youchann deleted the modify-dataTable branch July 20, 2020 07:03
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.

None yet

2 participants