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

Feature request - tooltip minimal prop and flexibility over arrow #4374

Merged
merged 10 commits into from
Oct 23, 2020

Conversation

jamesgiu
Copy link
Contributor

Fixes #0000

Checklist

  • Includes tests
  • Update documentation (auto-generated)

Changes proposed in this pull request:

Flexibility over rendering the arrow was a use case required for our own project - however, I also noticed that Popover's minimal prop exhibited this same behaviour so I thought I'd keep it consistent.

If minimal isn't quite right, I'd be happy to change this PR to just be a prop that controls the rendering of the arrow, as that was of importance to us in our use case.

Reviewers should focus on:

Whether minimal is necessary for tooltip given one can pass in Classes.MINIMAL via popoverProps. If not, propose prop solely controls rendering of the arrow.

Screenshot

image

Thank you for the consideration! Big fan of BlueprintJS

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

In general, I am ready to add this feature to Tooltip. I think we've heard of enough use cases for it that it makes sense.

Can you please update the docs example in tooltipExample.tsx to demonstrate usage of this new feature?

* transitions.
* @default false
*/
minimal?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be moved from IPopoverProps in popover.tsx to IPopoverSharedProps in popoverSharedProps.ts. Then you don't have to add it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @adidahiya - thanks for your feedback! Commit ab11371fb3fe26d67c63f6e0934a869d023689b0 addresses your suggestions.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

minor docs language suggestion

packages/core/src/components/popover/popoverSharedProps.ts Outdated Show resolved Hide resolved
@adidahiya adidahiya merged commit 38ab4df into palantir:develop Oct 23, 2020
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.

2 participants