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

[core] feat(Tabs): new 'fill' prop #5961

Merged
merged 8 commits into from
Feb 27, 2023
Merged

Conversation

CPerinet
Copy link
Contributor

@CPerinet CPerinet commented Feb 20, 2023

2023-02-27 18 17 18

@@ -285,9 +293,8 @@ export class Tabs extends AbstractPureComponent2<TabsProps, ITabsState> {

let indicatorWrapperStyle: React.CSSProperties = { display: "none" };
if (selectedTabElement != null) {
const { clientHeight, clientWidth, offsetLeft, offsetTop } = selectedTabElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clientHeight is not needed thanks to align-self: stretch; on the tab

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we still need this for vertical tabs.

@@ -482,10 +482,6 @@
margin-top: $pt-grid-size * 0.5;
}

.docs-tabs-example .#{$ns}-navbar .#{$ns}-tab {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of override that wont be necessary after this PR

@@ -103,6 +111,10 @@ $tab-indicator-width: 3px !default;
position: relative;
vertical-align: top;

display: flex;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the dangerous bit. If the tab is used with unwrapped content, flex will apply to it. It will align content on a row and vertically centred.
It removes the need for using line-height making the fill implementation much nicer and will allow for adding icon and tag prop in this pr

@CPerinet CPerinet marked this pull request as ready for review February 20, 2023 16:13
Charles Perinet added 2 commits February 20, 2023 19:18
color: $pt-text-color;
cursor: pointer;
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this does cause some issues for existing code which is inserting tags into tab titles (red border added for extra clarity):

Edit: nevermind, we're good. the align-items style seems to fix things

Before After (without align-items: center) After (with align-items: center)
image image image

the code for this is something like:

<Tab
  title={
    <>
      Pull requests
      {" "}
      <span className={classNames(Classes.TAG, Classes.MINIMAL, Classes.ROUND)}>0</span>
    </>
  }
/>

@adidahiya adidahiya changed the title Tabs fill height [core] feat(Tabs): new 'fill' prop Feb 27, 2023
@adidahiya adidahiya merged commit a838eb3 into palantir:develop Feb 27, 2023
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