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

fix: table 内 th の詳細度が高すぎる #802

Closed
wants to merge 3 commits into from

Conversation

uknmr
Copy link
Collaborator

@uknmr uknmr commented May 25, 2020

tableth の詳細度が高すぎるため、スタイルのオーバーライドが面倒になってる。
厳密に table > thead > tr > th である必要はまったくない。

これが table > thead > tr > th
こうなる table th

@uknmr uknmr requested a review from a team as a code owner May 25, 2020 00:11
@uknmr uknmr requested review from nabeliwo and removed request for a team May 25, 2020 00:11
@netlify
Copy link

netlify bot commented May 25, 2020

Deploy preview for smarthr-ui ready!

Built with commit 458636f

https://deploy-preview-802--smarthr-ui.netlify.app

Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

1件コメントしました!

@@ -34,7 +34,7 @@ const Wrapper = styled.table<{ themes: Theme }>`
border-spacing: 0;
background-color: ${COLUMN};

& > thead > tr > th {
th {
Copy link
Member

Choose a reason for hiding this comment

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

[ask] これまで thead 内の th にしか当たらなかったスタイルが tbody 内の th にも当たるように挙動が変わりましたがデザイン的にはそれで問題ないでしょうか?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

それはヘッダが縦に並ぶ場合の話しだと思うんですが、あくまで th は table header であり、この th への background-color はその table header を表すものだと解釈したので問題ないと思っています!

Copy link
Member

Choose a reason for hiding this comment

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

であれば問題ないです!

nabeliwo
nabeliwo previously approved these changes May 25, 2020
Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@@ -34,7 +34,7 @@ const Wrapper = styled.table<{ themes: Theme }>`
border-spacing: 0;
background-color: ${COLUMN};

& > thead > tr > th {
th {
Copy link
Member

Choose a reason for hiding this comment

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

であれば問題ないです!

Copy link
Member

@nabeliwo nabeliwo left a comment

Choose a reason for hiding this comment

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

遅れました!すみません! LGTM 🎉

@uknmr
Copy link
Collaborator Author

uknmr commented May 31, 2020

フォークして PR してたので出し直し。
#816

@uknmr uknmr closed this May 31, 2020
@uknmr uknmr deleted the fix-table-head-cell branch May 31, 2020 18:34
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