#2170 Toolbar (toolbarConfig) to support Toggletip, and it should popup like tooltip (shows up when mouse hover on it) #2171

mikieyx · 2024-09-23T16:27:35Z

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: Michael Pavlik <[email protected]>

…r-toggletip

Signed-off-by: Michael Pavlik <[email protected]>

tomlyn · 2024-09-23T17:07:00Z

canvas_modules/common-canvas/src/tooltip/tooltip.scss

@@ -30,7 +30,7 @@
 	line-height: 1.2;
 	text-align: left;
 	z-index: 10000; // Modal layout has z-index 9000. Show tooltip on top of modal.
-	pointer-events: none;
+	// pointer-events: none;


If this is not needed it should be deleted not commented out. (Note: There shouldn't be any commented out code merged into main unless there is a comment to indicate why the code is commented out.)

tomlyn · 2024-09-23T17:11:52Z

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

-	showToolTipOnClick: PropTypes.bool
+	showToolTipOnClick: PropTypes.bool,
+	hoverable: PropTypes.bool, // if true, can hover over to the tooltip, instead of immedieatly disappearing.
+	inTooltip: PropTypes.bool


You are declaring inTooltip here as a prop for the Tooltip object but you are using it as this.inTooltip. There's a difference between a prop and member variables of a React object. You should remove this from here and declare this.inTooltip in the constructor -- with a comment to explain the purpose of the variable. (Note: it's not strictly necessary to declare it, since it will default to undefined (falsey) when you first use it, but declaring in in the constructor given you the opportunity to comment on its usage)

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

tomlyn · 2024-09-23T17:16:55Z

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

@@ -154,7 +154,6 @@ class ToolTip extends React.Component {
 	}

 	showTooltipWithDelay() {
-


Unless you have a really good reason, you should avoid removing (or adding) empty lines in the code you are not altering as part of your PR.

tomlyn · 2024-09-23T17:18:22Z

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

@@ -356,7 +355,19 @@ class ToolTip extends React.Component {
 		if (this.props.children) {
 			// when children are passed in, tooltip will handle show/hide, otherwise consumer has to hide show/hide tooltip
 			const mouseover = () => this.setTooltipVisible(true);
-			const mouseleave = () => this.setTooltipVisible(false);
+			// const mouseleave = () => this.setTooltipVisible(false);


Code that i snot needed should be removed not commented out. (Note: There shouldn't be any commented out code merged into main unless there is a comment to indicate why the code is commented out.)

Signed-off-by: Michael Pavlik <[email protected]>

tomlyn

I made some minor comments.

tomlyn · 2024-09-25T17:05:30Z

canvas_modules/harness/src/client/App.js

+									<Button kind="danger" size="sm" style={ { width: "30px", height: "10px" } }>Reload</Button>
+								</div>
+							</div>
+					}


Can you pull out this JSX for the tooltip and assign it to a variable above (like the way subMenuTextSize and subMenuSize are created and then just include the variable here?

tomlyn · 2024-09-25T17:06:46Z

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

@@ -35,6 +35,7 @@ class ToolTip extends React.Component {
 		this.tabKeyPressed = false;
 		// Tooltip should not close if link inside tooltip is clicked.
 		this.linkClicked = false;
+		this.inTooltip = false; // A boolean variable that determines if the cursor is in the tooltip


Sorry to be picky -- this should say '... cursor is over the tooltip'

tomlyn · 2024-09-25T17:08:59Z

canvas_modules/common-canvas/src/tooltip/tooltip.jsx

@@ -522,14 +541,16 @@ ToolTip.propTypes = {
 	showToolTipIfTruncated: PropTypes.bool, // Set to true to only display tooltip if full text does not fit in displayable width
 	truncatedRef: PropTypes.object,
 	delay: PropTypes.number,
-	showToolTipOnClick: PropTypes.bool
+	showToolTipOnClick: PropTypes.bool,
+	hoverable: PropTypes.bool, // If true, can hover over to the tooltip, instead of immediately disappearing.


Sorry picky again. Please change to: "If true, mouse cursor can be hovered over to the tooltip."

Signed-off-by: Michael Pavlik <[email protected]>

…olbar-toggletip

tomlyn

LGTM

Michael Pavlik added 3 commits September 23, 2024 09:10

made tooltip hoverable

8f7a9c0

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'main' of https://github.com/elyra-ai/canvas into toolba…

9d2e90b

…r-toggletip

clean up code

447df33

Signed-off-by: Michael Pavlik <[email protected]>

mikieyx requested a review from tomlyn September 23, 2024 16:29

tomlyn requested changes Sep 23, 2024

View reviewed changes

Michael Pavlik and others added 3 commits September 23, 2024 12:55

fixing up requests

cbf78af

Signed-off-by: Michael Pavlik <[email protected]>

added comment

f83332d

Signed-off-by: Michael Pavlik <[email protected]>

adding an example in test harness

e43fe3a

Signed-off-by: Michael Pavlik <[email protected]>

mikieyx requested a review from tomlyn September 25, 2024 16:33

Merge branch 'main' into toolbar-toggletip

747fc31

tomlyn requested changes Sep 25, 2024

View reviewed changes

Michael Pavlik added 2 commits September 25, 2024 10:45

making minor changes

5943192

Signed-off-by: Michael Pavlik <[email protected]>

Merge branch 'toolbar-toggletip' of github.com:mikieyx/canvas into to…

406e5c8

…olbar-toggletip

mikieyx requested a review from tomlyn September 25, 2024 17:46

tomlyn approved these changes Sep 25, 2024

View reviewed changes

Merge branch 'main' into toolbar-toggletip

4bf7814

tomlyn merged commit 187b348 into elyra-ai:main Sep 26, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2170 Toolbar (toolbarConfig) to support Toggletip, and it should popup like tooltip (shows up when mouse hover on it) #2171

#2170 Toolbar (toolbarConfig) to support Toggletip, and it should popup like tooltip (shows up when mouse hover on it) #2171

mikieyx commented Sep 23, 2024

tomlyn Sep 23, 2024

tomlyn Sep 23, 2024

tomlyn Sep 23, 2024

tomlyn Sep 23, 2024

tomlyn left a comment

tomlyn Sep 25, 2024

tomlyn Sep 25, 2024

tomlyn Sep 25, 2024

tomlyn left a comment

		@@ -154,7 +154,6 @@ class ToolTip extends React.Component {
		}

		showTooltipWithDelay() {

#2170 Toolbar (toolbarConfig) to support Toggletip, and it should popup like tooltip (shows up when mouse hover on it) #2171

#2170 Toolbar (toolbarConfig) to support Toggletip, and it should popup like tooltip (shows up when mouse hover on it) #2171

Conversation

mikieyx commented Sep 23, 2024

tomlyn Sep 23, 2024

Choose a reason for hiding this comment

tomlyn Sep 23, 2024

Choose a reason for hiding this comment

tomlyn Sep 23, 2024

Choose a reason for hiding this comment

tomlyn Sep 23, 2024

Choose a reason for hiding this comment

tomlyn left a comment

Choose a reason for hiding this comment

tomlyn Sep 25, 2024

Choose a reason for hiding this comment

tomlyn Sep 25, 2024

Choose a reason for hiding this comment

tomlyn Sep 25, 2024

Choose a reason for hiding this comment

tomlyn left a comment

Choose a reason for hiding this comment