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.

Sign up for GitHub

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

Jump to bottom

テーブル周りのborder修正 #34

Merged
merged 9 commits into from
May 27, 2020
Merged

テーブル周りのborder修正 #34

merged 9 commits into from
May 27, 2020

Conversation

youchann
Copy link
Contributor

No description provided.

@youchann youchann self-assigned this May 22, 2020
youchann
youchann commented May 22, 2020
View reviewed changes
src/components/DataTable/internal/Table/Header.tsx Outdated
Comment on lines 8 to 12
const Component = styled.thead`
border-top: ${Size.Border.Small} solid ${colors.basic[300]};
border-bottom: ${Size.Border.Small} solid ${colors.basic[300]};
background-color: ${colors.basic[100]};
`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<thead />にborderとbackground-colorを持たせたのでコード量が少し減っている

src/components/DataTable/internal/TableTabs/styled.ts Outdated
@@ -19,15 +17,11 @@ export const TabItem = styled.li<{ active: boolean; width: string }>`
margin-bottom: -${Size.Border.Small}; /* containerのborderにかぶせるためのネガティブマージン */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

あ、ここのおかげでborderが塗りつぶされていたのか...!!

src/components/DataTable/internal/TableTabs/styled.ts Outdated
Comment on lines 20 to 24
border-left: ${Size.Border.Small} solid ${colors.basic[300]};
border-top: ${Size.Border.Small} solid ${colors.basic[300]};
border-right: ${Size.Border.Small} solid ${colors.basic[300]};
border-bottom: ${({ active }) =>
active ? `none` : `${Size.Border.Small} solid ${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.

basic[300]がpalette上にないんですよね。。。

theme.palette.borderみたいなの作った方が良い気がしている。

@youchann youchann requested a review from kinokoruumu May 22, 2020 06:08
@kinokoruumu
Copy link
Contributor

@yutaro1031
変更前と変更後のスクショとか貼ってもらえるとレビューしやすい！

@youchann
Copy link
Contributor Author

@kinokoruumu
なるほど、用意します！！

https://github.com/voyagegroup/fluct_XDC/issues/28
↑スクショと同じもの※外部の人には見れない

@youchann
Copy link
Contributor Author

普通のテーブル

before
image

after
image

@youchann
Copy link
Contributor Author

Datatable

before

image

after

image

@youchann
Copy link
Contributor Author

@kinokoruumu スクショとりました(とてもみやすい...

kinokoruumu
kinokoruumu reviewed May 27, 2020
View reviewed changes
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.

めっちゃ見やすくなった！

src/components/DataTable/internal/Table/Header.tsx Outdated

export type Props = {};

const Component = styled.thead`
border-top: ${Size.Border.Small} solid ${colors.basic[300]};
Copy link
Contributor

Choose a reason for hiding this comment

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

これはgray.lightをbasic[300]に変えるのではなく、テーブルだけボーダーを濃くしたいってことで大丈夫かな？

src/components/MenuList/internal/Divider/styled.ts Outdated

export const Divider = styled.hr`
border: none;
margin: 4px 0;
height: 2px;
background-color: ${colors.basic[200]};
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.

Issueには書かれていないけど、paletteから参照できそうだったのでした。という感じですね!!

ただ、dropdownButtonはRevertされたので、その変更自体も亡くなっております!!

youchann
youchann commented May 27, 2020
View reviewed changes
Copy link
Contributor Author

@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.

backgroundにcolors.basic[300]が適用されている箇所はいくつかあるのですが、それらにはdividerを適用していません。

src/themes/palette.ts
@@ -94,6 +95,7 @@ export const palette: Palette = {
active: colors.blue[100],
hint: colors.blue[50] as string, // TODO
},
divider: 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.

divider爆誕

@youchann youchann requested a review from kinokoruumu May 27, 2020 06:45
kinokoruumu
kinokoruumu approved these changes May 27, 2020
View reviewed changes
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.

コメしたとこ以外はLGTMです！

src/components/DataTable/internal/Table/Header.tsx Outdated
border-top: ${Size.Border.Small} solid ${({ theme }) => theme.palette.divider};
border-bottom: ${Size.Border.Small} solid
${({ theme }) => theme.palette.divider};
background-color: ${colors.basic[100]};
Copy link
Contributor

Choose a reason for hiding this comment

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

gray.highlight使ってもいい気がする

@youchann youchann merged commit 27d2feb into master May 27, 2020
@youchann youchann deleted the tweak-datatable branch May 27, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinokoruumu kinokoruumu kinokoruumu approved these changes

Assignees

@youchann youchann

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@youchann @kinokoruumu