Skip to content

Conversation

@brianferry
Copy link
Collaborator

@brianferry brianferry commented Jun 16, 2022

What I Did

  • Remove all scss specific styling and implement the css styling from patternfly.org
  • Implement many of the --pf-c- variables from patternfly.

Choices

  • Did not implement the full API from patternfly tooltip and instead used a slim version that will be compatible with rhds tooltip
  • Did not style the invoker directly and instead left that up to the lightdom as it will not always be the case that the invoker is an icon or button element.

Note To Reviewers

  • Please take a look at the stylings related to dark theme and make sure that aligns with the expectations for rhds. Going from the docs it does but when content is partially dark theme and partially light theme it isn't as clear.

brianferry and others added 21 commits March 22, 2022 23:20
@brianferry brianferry added the demo Updating demo pages label Jun 16, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2022

🦋 Changeset detected

Latest commit: 5d18f1a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@patternfly/pfe-tooltip Minor
@patternfly/pfe-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added work in progress POC / Not ready for review docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools AT passed Automated testing has passed labels Jun 16, 2022
@bennypowers
Copy link
Member

@brianferry do make sure to squash when merging, please

@brianferry brianferry enabled auto-merge (squash) July 26, 2022 13:11
@brianferry brianferry requested review from marionnegp and removed request for marionnegp July 26, 2022 13:11
Copy link
Contributor

@marionnegp marionnegp left a comment

Choose a reason for hiding this comment

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

@brianferry, the changes look good! I added a comment about the background color.

Can the tooltip be shifted so that the arrow is centered on the invoker for the following tooltip positions?
Screen Shot 2022-07-26 at 11 00 24 AM

  • Left-Start Tooltip
  • Left-End Tooltip
  • Right-Start Tooltip
  • Right-End Tooltip
  • Top-Start Tooltip
  • Bottom-Start Tooltip

The top-end and bottom-end positioning look correct.

@bennypowers
Copy link
Member

@brianferry, the changes look good! I added a comment about the background color.

Can the tooltip be shifted so that the arrow is centered on the invoker for the following tooltip positions? Screen Shot 2022-07-26 at 11 00 24 AM

* [ ]  Left-Start Tooltip

* [ ]  Left-End Tooltip

* [ ]  Right-Start Tooltip

* [ ]  Right-End Tooltip

* [ ]  Top-Start Tooltip

* [ ]  Bottom-Start Tooltip

The top-end and bottom-end positioning look correct.

the start and end positions there indicate where along the desired edge the arrow should appear, so these are correct afaict

@marionnegp
Copy link
Contributor

marionnegp commented Jul 26, 2022

@bennypowers, I thought so too, but they look centered for the top-end and bottom-end tooltips. Is there a way to get the other tooltips to be positioned similarly?

Screen Shot 2022-07-26 at 11 19 01 AM

I'm not able to see these variations in relation to the invoker on patternfly.org though, so if it already matches what's on PF, it's fine.

@brianferry
Copy link
Collaborator Author

@bennypowers, I thought so too, but they look centered for the top-end and bottom-end tooltips. Is there a way to get the other tooltips to be positioned similarly?

Screen Shot 2022-07-26 at 11 19 01 AM

I'm not able to see these variations in relation to the invoker on patternfly.org though, so if it already matches what's on PF, it's fine.

I think this might be a case of where our demos don't show a button like patternfly.org so it looks different because of the size of the element. I set up an element locally and top-start / top-end looks like this

image

Which seems to lineup with how patternfly does it

image

@bennypowers
Copy link
Member

bennypowers commented Jul 26, 2022

@brianferry wdyt about updating those examples in the demo, for clarity? and thanks @marionnegp for clarifying the issue

@brianferry
Copy link
Collaborator Author

@brianferry wdyt about updating those examples in the demo, for clarity? and thanks @marionnegp for clarifying the issue

I think that's a good idea, just pushed an update which should add the buttons to the demos instead of the icons for clarity. @marionnegp - tagging for visibility.

@brianferry brianferry disabled auto-merge July 26, 2022 19:03
@brianferry brianferry merged commit 7c9b85c into main Jul 26, 2022
@brianferry brianferry deleted the feat/pfe-tooltip branch July 26, 2022 19:03
@bennypowers
Copy link
Member

Well done, @brianferry and reviewers!

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

Labels

1:1 Aligning components with PatternFly v4 AT passed Automated testing has passed demo Updating demo pages docs Documentation updates functionality Functionality, typically pertaining to the JavaScript. ready: code review Ready for code review! styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[1:1] pfe-tooltip

6 participants