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.

Sign up for GitHub

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

Jump to bottom

CalciteTooltip | CalcitePopover Enhancements #167

Merged
merged 51 commits into from
Oct 15, 2019
Merged

CalciteTooltip | CalcitePopover Enhancements #167

merged 51 commits into from
Oct 15, 2019

Conversation

driskull
Copy link
Member

@driskull driskull commented Oct 9, 2019 •
edited
Loading

calcite-tooltip

Properties

Property Attribute Description Type Default
open open Display and position the component. boolean false
placement placement Determines where the component will be positioned relative to the referenceElement. "auto" | "auto-end" | "auto-start" | "bottom" | "bottom-end" | "bottom-start" | "left" | "left-end" | "left-start" | "right" | "right-end" | "right-start" | "top" | "top-end" | "top-start" "auto"
referenceElement reference-element Reference HTMLElement used to position this component. HTMLElement | string undefined
theme theme Select theme (light or dark) "dark" | "light" "light"

Methods

reposition() => Promise<void>

Returns

Type: Promise<void>

#155 (comment)

Related to #155

calcite-popover

Properties

Property Attribute Description Type Default
addClickHandle add-click-handle Adds a click handler to the referenceElement to toggle open the Popover. boolean false
closeButton close-button Display a close button within the Popover. boolean false
open open Display and position the component. boolean false
placement placement Determines where the component will be positioned relative to the referenceElement. horizontal: Positioned to the left or right of the referenceElement. vertical: Positioned above or below the referenceElement. "horizontal" | "vertical" "horizontal"
referenceElement reference-element Reference HTMLElement used to position this component according to the placement property. HTMLElement | string undefined
xOffset x-offset Offset the position of the popover in the horizontal direction. number 0
yOffset y-offset Offset the position of the popover in the vertical direction. number 0

Methods

reposition() => Promise<void>

Returns

Type: Promise<void>

toggle() => Promise<void>

Returns

Type: Promise<void>

Slots

Slot Description
"image" A slot for adding an image. The image will appear above the other slot content.

Built with StencilJS

driskull and others added 30 commits October 3, 2019 10:53
Styling updates
a73bd37 
- removed hover and pressed states from tooltip
- updated `x` location and used flexbox
- general styling updates
@julio8a julio8a removed the help wanted Issues that the core team needs help with in a sprint. label Oct 9, 2019
@macandcheese
Copy link
Contributor

macandcheese commented Oct 9, 2019 •
edited
Loading

New stuff looks great! Couple of questions for @julio8a

Should we just provide padding OOTB for folks on the popover div? Say, 12px by default for x and y padding to match that of tooltip? Otherwise we open it up for folks to use whatever inconsistent padding they want...

Should we flex close button full height of container to prevent weird non-clickable "not quite as tall" areas like this? I guess it's kind of an inevitability unless we want center aligned close buttons for lots of text, but it sure is weird...

Screen Shot 2019-10-09 at 4 48 35 PM

Now that there are no border radius on these, the x shouldn't have border-radius either. I think they previously did in the tooltip component but removing it as we did now solves, so the close button should also have 0px border radius now that that has changed.

@driskull
Copy link
Member Author

Should we just provide padding OOTB for folks on the popover div? Say, 12px by default for x and y padding to match that of tooltip? Otherwise we open it up for folks to use whatever inconsistent padding they want...

For the Map viewer, they would like to control the padding I think. They are putting other components inside it that already have padding defined. For the tooltip, i'm ok with default padding.

Julio Ochoa and others added 2 commits October 10, 2019 09:20
Removed border radius on container and close button
9e8b188
@macandcheese
Updates css, removes stale classes
5d22e3d 
Moves font size declaration to container, adds component prefix, removes unused classes.
@vcolavin
Copy link
Contributor

https://en.wikipedia.org/wiki/Popover

@driskull driskull changed the title CalcitePopover Enhancements CalciteTooltip | CalcitePopover Enhancements Oct 10, 2019
@driskull driskull mentioned this pull request Oct 10, 2019
Closed
@driskull
Copy link
Member Author

Tooltip has been made accessible.

@julio8a
Copy link

julio8a commented Oct 10, 2019

tenor

@driskull
Copy link
Member Author

Accessibility fixes installed for Popover and Tooltip but could use review from some folks. Thanks!

@driskull driskull added the new component Issues tied to a new component. label Oct 11, 2019
julio8a
julio8a approved these changes Oct 14, 2019
View reviewed changes
Copy link

@julio8a julio8a left a comment

Choose a reason for hiding this comment

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

I can't review the accessibility work, but desing looks good!

@driskull
Copy link
Member Author

Is this good to merge?

@driskull
Copy link
Member Author

Going to merge this. We can create follow up issues for anything else needed.

@driskull driskull merged commit 396d3fa into master Oct 15, 2019
@driskull driskull deleted the dris0000/calcite-popover-enhancements branch October 15, 2019 15:52
@macandcheese macandcheese added this to the 🚢 v1-beta.11 milestone Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vcolavin vcolavin vcolavin approved these changes

@julio8a julio8a julio8a approved these changes

@macandcheese macandcheese Awaiting requested review from macandcheese

@patrickarlt patrickarlt Awaiting requested review from patrickarlt

@jcfranco jcfranco Awaiting requested review from jcfranco

Assignees

@driskull driskull

Labels
enhancement Issues tied to a new feature or request. new component Issues tied to a new component.
Projects
None yet
Milestone
v1.0.0-beta.11
Development

Successfully merging this pull request may close these issues.
4 participants
@driskull @macandcheese @vcolavin @julio8a