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

Tweak layout spacing for the timetable screen tabs #911

Merged

Conversation

h6ah4i
Copy link
Contributor

@h6ah4i h6ah4i commented Aug 23, 2023

Overview (Required)

Tweak spacing around tabs on the timetable screen.

Links

Screenshot

Before After

@h6ah4i h6ah4i requested a review from a team as a code owner August 23, 2023 10:57
@h6ah4i h6ah4i requested a review from tkhs0604 August 23, 2023 10:57
@github-actions
Copy link

Test Results

179 tests   179 ✔️  6m 23s ⏱️
  11 suites      0 💤
  11 files        0

Results for commit 9265e8f.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 23, 2023 13:06 Inactive
Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

I'm not sure how it calculated a lot. But looks great!

private val maxTabRowHeight = 84.dp
private val minTabRowHeight = 56.dp
private val tabIndicatorHorizontalSpacing = 8.dp
private val tabRowHorizontalSpacing = (maxTabRowHeight - baselineTabHeight) / 2 - tabIndicatorHorizontalSpacing
Copy link
Member

Choose a reason for hiding this comment

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

May I ask why we used maxTabRowHeight - baselineTabHeight here? Is it a size for the corner round?

Copy link
Contributor Author

@h6ah4i h6ah4i Aug 23, 2023

Choose a reason for hiding this comment

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

Let me explain 🙋‍♂️

Tabs are already vertically center aligned to the parent container element (TabRow), so I thought the horizontal spacing size should be the same as the vertical ones.

However, I noticed my mistake after this PR getting merged. (Sorry!) I checked the UI design spec (Figma) and it says a bit different layout constraints specified to Tabs and TabRow. They are not vertically aligned and the bottom padding amount is less than other directions.

I'm going to create a PR to match the implementation with the UI design spec later.

g112452

Copy link
Member

Choose a reason for hiding this comment

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

Now I see the reason we use height!

@takahirom takahirom merged commit 281ff65 into DroidKaigi:main Aug 23, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants