Bug 1807204: Update cursor to show drop status over targets #4486

jeff-phillips-18 · 2020-02-25T21:15:17Z

Fixes:
https://issues.redhat.com/browse/ODC-3028

Analysis / Root cause:
The cursor was not updated during drag operations to indicate drop status.

Solution Description:
Modified the cursor to:

Use an SVG for the Add cursor when creating connections
Show a tooltip when over a valid add drop target informing the user that dropping will add an application here (after the standard delay without a mouse move)
Show an invalid drop cursor when over items where a drop would do nothing

** Sample Screen Shots **

Browser conformance:

Chrome
Firefox
Safari
Edge

cc @openshift/team-devconsole-ux

/kind bug

openshift-ci-robot · 2020-02-25T21:15:24Z

@jeff-phillips-18: This pull request references Bugzilla bug 1807204, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1807204: Update cursor to show drop status over targets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

serenamarie125

LGTM! @Veethika can you take a look as well?

jeff-phillips-18 · 2020-02-25T22:01:08Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-02-25T22:01:09Z

@jeff-phillips-18: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

christianvogt · 2020-02-25T23:31:44Z

frontend/packages/topology/src/behavior/withCreateConnector.tsx

+  React.useEffect(() => {
+    setHover(false);
+    clearTimeout(unsetHandle.current);
+    unsetHandle.current = window.setTimeout(() => setHover(true), 2000);
+    return () => {
+      clearTimeout(unsetHandle.current);
+    };
+  }, [endPoint.x, endPoint.y]);


This is better off going directly into the DefaultCreateConnector.

In dev-console we can supply our own prop to renderConnector which uses DefaultCreateConnector to supply the custom message.

christianvogt · 2020-02-25T23:34:14Z

frontend/packages/dev-console/src/components/topology/components/nodes/KnativeServiceGroup.tsx

+      content="Move sink to service"
+      trigger="manual"
+      isVisible={dropTarget && canDrop}
+      tippyProps={{ duration: 0, delay: 0 }}


Can you confirm the behavior with UX? 0 delay will bring up tooltips immediately as you hover your mouse over an intermediate node.

Didn't we try avoid this kind of behavior for label truncation by having a small delay.

UX spec specifies For connectors, the tooltip should appear the moment the user hovers over a valid drop target.

christianvogt · 2020-02-25T23:37:06Z

frontend/packages/dev-console/src/components/topology/componentUtils.ts

+  collect: (monitor) => {
+    const dragEditInProgress =
+      monitor.isDragging() && editOperations.includes(monitor.getOperation());
+    const dropHints = monitor.getDropHints() || [];


maybe we should change getDropHints to always return an array (string[])?

jeff-phillips-18 · 2020-02-26T21:01:41Z

Updated per comments.

Removed the grab cursor when dragging edges over valid drop nodes.

In order to move out the dev-console specific hover treatment for the create connector, I had to refactor a bunch to make the default connector a component rather than a function call that returns a component.

christianvogt · 2020-02-26T21:59:25Z

frontend/packages/topology/src/components/CreateConnectorWidget.tsx

+  choices: (ConnectorChoice | React.ReactElement)[];
+};
+
+export const CreateConnectorWidget: React.FC<CreateConnectorWidgetProps> = observer((props) => {


Prefer a single default export for component files.

christianvogt · 2020-02-26T22:11:07Z

frontend/packages/topology/src/components/CreateConnectorWidget.tsx

+export const CREATE_CONNECTOR_OPERATION = 'createconnector';
+export const CREATE_CONNECTOR_DROP_TYPE = '#createConnector#';


Old code but:

Suggested change

export const CREATE_CONNECTOR_OPERATION = 'createconnector';

export const CREATE_CONNECTOR_DROP_TYPE = '#createConnector#';

export const CREATE_CONNECTOR_OPERATION = '#createconnector#';

export const CREATE_CONNECTOR_DROP_TYPE = '#createConnector#';

christianvogt · 2020-02-26T22:19:40Z

frontend/packages/topology/src/components/CreateConnectorWidget.tsx

+export type CreateConnectorRendererProps = {
+  startPoint: Point;
+  endPoint: Point;
+  dragging: boolean;
+  hints: string[] | undefined;
+  tipContents?: React.ReactNode;
+  className?: string;
+};
+
+export type CreateConnectorRenderer = React.ComponentType<CreateConnectorRendererProps>;


This isn't correct. The only required props that are expected of a component used in withCreateConnector are:

startPoint: Point, endPoint: Point, hints: string[] | undefined,

christianvogt · 2020-02-26T22:20:44Z

frontend/packages/topology/src/components/DefaultCreateConnector.tsx


-const DefaultCreateConnector: React.FC<DefaultCreateConnectorProps> = ({
+const DefaultCreateConnector: CreateConnectorRenderer = ({


You'll need a new prop type that includes support for tipContents and className.

christianvogt · 2020-02-26T22:21:55Z

frontend/packages/topology/src/components/index.ts

@@ -5,3 +5,5 @@ export { default as ConnectorArrow } from './ConnectorArrow';
 export { default as EdgeConnectorArrow } from './EdgeConnectorArrow';
 export { default as GraphComponent } from './GraphComponent';
 export { default as VisualizationSurface } from './VisualizationSurface';
+export * from './CreateConnectorWidget';


Do not expose contents of CreateConnectorWidget as API. This is for internal use only to withCreateConnector. If needed, re-export any specific types from withCreateConnector.

openshift-ci-robot · 2020-02-26T22:24:09Z

@jeff-phillips-18: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/verify	`41c16e9`	link	`/test verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2020-02-26T23:29:13Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

openshift-ci-robot · 2020-02-26T23:29:16Z

@openshift-bot: This pull request references Bugzilla bug 1807204, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

christianvogt · 2020-02-27T04:14:58Z

/lgtm
/approve

openshift-ci-robot · 2020-02-27T04:15:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18, serenamarie125

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt]
~~frontend/packages/topology/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-02-27T05:47:30Z

@jeff-phillips-18: All pull requests linked via external trackers have merged. Bugzilla bug 1807204 has been moved to the MODIFIED state.

In response to this:

Bug 1807204: Update cursor to show drop status over targets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2020-02-27T05:47:45Z

@jeff-phillips-18: new pull request created: #4525

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Feb 25, 2020

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Feb 25, 2020

openshift-ci-robot requested review from invincibleJai and spadgett February 25, 2020 21:16

serenamarie125 approved these changes Feb 25, 2020

View reviewed changes

christianvogt reviewed Feb 25, 2020

View reviewed changes

jeff-phillips-18 added 2 commits February 26, 2020 11:39

Bug 1807204: Update cursor to show drop status over targets

128472c

Updates per review comments

af1fa27

jeff-phillips-18 force-pushed the cursor-updates branch from 41c16e9 to af1fa27 Compare February 26, 2020 20:59

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2020

christianvogt reviewed Feb 26, 2020

View reviewed changes

Remove refactor to CreateConnectorWidget component

97b73bc

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 26, 2020

openshift-ci-robot assigned christianvogt Feb 27, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2020

openshift-merge-robot merged commit 3033aeb into openshift:master Feb 27, 2020

openshift-cherrypick-robot mentioned this pull request Feb 27, 2020

[release-4.4] Bug 1807861: Update cursor to show drop status over targets #4525

Merged

spadgett added this to the v4.5 milestone Feb 28, 2020

jeff-phillips-18 deleted the cursor-updates branch December 2, 2020 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1807204: Update cursor to show drop status over targets #4486

Bug 1807204: Update cursor to show drop status over targets #4486

jeff-phillips-18 commented Feb 25, 2020

openshift-ci-robot commented Feb 25, 2020

serenamarie125 left a comment

jeff-phillips-18 commented Feb 25, 2020

openshift-cherrypick-robot commented Feb 25, 2020

christianvogt Feb 25, 2020

christianvogt Feb 25, 2020

jeff-phillips-18 Feb 26, 2020

christianvogt Feb 25, 2020

jeff-phillips-18 commented Feb 26, 2020

christianvogt Feb 26, 2020

christianvogt Feb 26, 2020

christianvogt Feb 26, 2020

christianvogt Feb 26, 2020

christianvogt Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020 •

edited

openshift-bot commented Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020

christianvogt commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

		export const CREATE_CONNECTOR_OPERATION = 'createconnector';
		export const CREATE_CONNECTOR_DROP_TYPE = '#createConnector#';


		const DefaultCreateConnector: React.FC<DefaultCreateConnectorProps> = ({
		const DefaultCreateConnector: CreateConnectorRenderer = ({

Bug 1807204: Update cursor to show drop status over targets #4486

Bug 1807204: Update cursor to show drop status over targets #4486

Conversation

jeff-phillips-18 commented Feb 25, 2020

openshift-ci-robot commented Feb 25, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Feb 25, 2020

openshift-cherrypick-robot commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 26, 2020 • edited

openshift-bot commented Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020

christianvogt commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 26, 2020 •

edited