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

Add tooltip to preview disabled nodes #11048

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

zeusongit
Copy link
Contributor

Purpose

DYN-3074
Add description, out-ports, in-ports tooltip to nodes which had their preview disabled.

previewtooltip

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • The level of testing this PR includes is appropriate
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.

Reviewers

@DynamoDS/dynamo

Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

I'll approve, but I guess at some point we should change our approach for marking a node as preview-disabled. This is not the first issue we see regarding that semi transparent box added on top, preventing normal node usage. It probably makes more sense to simply change the colour of the node instead. Copying @mjkkirschner @aparajit-pratap @Amoursol to get some feedback.

@zeusongit zeusongit merged commit aa4eb9f into DynamoDS:master Aug 24, 2020
@Amoursol
Copy link
Contributor

Amoursol commented Aug 25, 2020

This was the problem submitted in the Problem Report, but I agree @mmisol that rather than bespoke solutions we should simply enable all node interactions when the Preview State is turned off with the exception of showing the Geometry in the background preview.

@aparajit-pratap
Copy link
Contributor

I'm not quite sure I understood why we needed to add another UI element for the tooltip when we already have it. Like @mmisol says, it should be a matter of keeping the existing tooltip enabled even in the preview-off state.

@zeusongit
Copy link
Contributor Author

zeusongit commented Aug 25, 2020

Well, the existing tooltip element was part of the rectangle, and disabling the preview adds a border on top of the node with its bg color set to light blue, and that did not have the tooltip element, which I added. I don't know if there is a way to create a separate tooltip element and bind it to both the rectangle and border.
And as @Amoursol said in standup today that right now the existing solution takes care of all the scenarios, but in future if we get some more update related to preview state we should look into extending the node to handle all the events without needing an extra layer on top.

@Amoursol
Copy link
Contributor

We can investigate these kind of issues in our Node Refresh work that is currently being explored by UX :)

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.

5 participants