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

[popover2] feat(Tooltip2): compact appearance #5822

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jan 4, 2023

Changes proposed in this pull request:

This PR introduces the compact prop for Tooltip2, which allows the components to render their content with smaller padding than the default appearance.

The new compact appearance is encouraged in data-dense UIs:

default compact
image image

I also noticed a minor typographic issue for <Code> spans inside text which stood out in the example for this new prop. The default vertical-align: baseline makes inline <Code> too low compared to its adjacent text, and I've found that a value of vertical-align: text-bottom looks better. Right now I'm conservatively applying this fix to the new .bp4-compact modifier class only.

Not in this PR (considered, but reverted)

For Popover2, compact={true} applies when popoverClassName={Popover2Classes.POPOVER2_CONTENT_SIZING} is set. The default 20px padding for this modifier class has often felt too large in practical usage, which forced Blueprint users to apply style overrides.

default compact
image image

@blueprint-bot
Copy link

[popover2] feat(Popover2, Tooltip2): compact appearance

Previews: documentation | landing | table | demo

}

// any descendant ContextMenu2s may update this ctxState
private renderPopover = (ctxState: Tooltip2ContextState) => {
const { children, disabled, intent, popoverClassName, ...restProps } = this.props;
const classes = classNames(
Classes.TOOLTIP2,
{ [CoreClasses.MINIMAL]: this.props.minimal },
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 line was intentionally removed. it's not necessary to apply this class here because Popover2 will do it anyway since we're passing through the minimal prop

@CPerinet
Copy link
Contributor

CPerinet commented Jan 5, 2023

Do we need it for Popover? Tooltips should only have a couple lines of text in it where Popover have more content. At the same time, compact mode make the spacing from elements to sides lower than between elements. This is "wrong" information architecture and end-up looking unbalanced.

@adidahiya
Copy link
Contributor Author

@CPerinet I found a bunch of popovers in our internal code bases with 10px padding, so I figured this compact modifier might be useful for those use cases. I did also find a handful of popovers with 5px or 15px padding (even some 12px, 8px...), so maybe this styling isn't as standard as I'd like it to be. I will have to think some more about this problem.

compact mode make the spacing from elements to sides lower than between elements. This is "wrong" information architecture and end-up looking unbalanced.

Good point... in the docs example this is an issue because the "footer" content section uses a 15px top margin:

image

For now, I'll revert the Popover2 changes and just keep this PR scoped to Tooltip2.

@blueprint-bot
Copy link

Revert Popover2 changes, scope to Tooltip2 only

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

reduce padding just a bit

Previews: documentation | landing | table | demo

@adidahiya adidahiya changed the title [popover2] feat(Popover2, Tooltip2): compact appearance [popover2] feat(Tooltip2): compact appearance Jan 5, 2023
@adidahiya
Copy link
Contributor Author

Iterated with @CPerinet over Slack, we've settled on a 5/7px padding for the compact tooltip. We may adjust this later if necessary.

@adidahiya adidahiya merged commit 7236e81 into develop Jan 5, 2023
@adidahiya adidahiya deleted the ad/tooltip2-compact branch January 5, 2023 15:58
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.

3 participants