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

use gap instead of margin for tab #6834

Merged
merged 3 commits into from
May 28, 2024
Merged

use gap instead of margin for tab #6834

merged 3 commits into from
May 28, 2024

Conversation

JosephChotard
Copy link
Contributor

@JosephChotard JosephChotard commented May 28, 2024

By doing this, tabs can natively support RTL without breaking any LTR

Changes proposed in this pull request:

Instead of using margin for tab spacing use flex gap

Reviewers should focus on:

  • Checking that it looks the same in both LTR
  • That it's no longer broken in RTL

Screenshot

Looks identical

@svc-palantir-github
Copy link

use gap instead of margin

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@svc-palantir-github
Copy link

made a mistake in 1 line PR

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@svc-palantir-github
Copy link

ugh alphabetical

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@JosephChotard
Copy link
Contributor Author

JosephChotard commented May 28, 2024

Existing view in RTL mode (notice margins break)
Screenshot 2024-05-28 at 16 39 39
New view:
Screenshot 2024-05-28 at 16 40 58

LTR looks identical
For these screenshots I added direction: rtl to a wrapper around the tab. We use RTL for Arabic / Hebrew text

@JosephChotard
Copy link
Contributor Author

Example in arabic.

Now:
Screenshot 2024-05-28 at 16 48 05

Before:
Screenshot 2024-05-28 at 16 49 18

Copy link
Contributor

@gluxon gluxon left a comment

Choose a reason for hiding this comment

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

Thank you!

Glad to see older CSS hacks cleaned up as gap for flexbox properties gets wider browser usage.

This might break if something overrides the .bp5-tab-list display property, but Blueprint consumers shouldn't be doing that anyway.

@gluxon gluxon merged commit 2893a00 into develop May 28, 2024
14 checks passed
@gluxon gluxon deleted the jc/make-tab-use-gap branch May 28, 2024 18:12
@stewartfeasby
Copy link

Hi

This completely breaks vertical tabbing now, which previously had no gap but now has massive gaps.

Thanks

@gluxon
Copy link
Contributor

gluxon commented Jul 1, 2024

Thanks for catching that @stewartfeasby. Putting up a fix here: #6885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants