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 timestamp to events, update positions on drag, and add clearOnUnhover prop. #195

Merged
merged 23 commits into from
Oct 18, 2023

Conversation

Farkites
Copy link
Contributor

@Farkites Farkites commented Sep 28, 2023

About

  • Update elements property when elements are modified in the Cytoscape UI
  • Add timestamp to tapNodeData and tapEdgeData properties such that they trigger a callback on every tap, even if the data itself has not changed
  • Add timestamp to mouseoverNodeData and mouseoverEdgeData properties such that they trigger a callback on every mouseover, even if the data itself has not changed
  • Update documentation
  • Update tests

This PR adds timestamps to some properties so they trigger callbacks every time the cytoscape events happen, adds property clearOnUnhover to configure the behaviour of mouseOver callbacks, and updates position of elements when they are dragged with the mouse.

Description of changes

Added

  • Clear clearOnUnhover property
  • Timestamp to tapNodeData and tapEdgeData properties
  • Timestamp to mouseoverNodeData and mouseoverEdgeData properties
  • Update elements positions when nodes are dragged

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Cytoscape core contributor.

Reference Issues

Closes #[issue number]

Other comments

cy.on('position', 'node', (event) => {
if (typeof this.props.setProps === 'function') {
this.props.setProps({
elements: this.props.elements.map((item) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Farkites Does this actually update the position of the elements? It looks like this would trigger a prop update with the new timestamp, but still using the old position of the elements.

But generally this feels cleaner -- so an approach like this is on the right track I think.

Looking into this a bit this AM.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I have an idea... it looks like ele.json() might give us something close to what we want... how about something like

                this.props.setProps({
                     elements: cy.elements('').map((item) => item.json()),
                 });

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what ele.json() returns:

Screen Shot 2023-10-04 at 11 18 19 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach works with:

this.props.setProps({
    elements: cy.elements('').map((item) => {
         return {
             data: item.json().data,
             position: item.json().position,
         };
    }),
});

Although it introduces position: {x: 0, y:0} for edges...

CHANGELOG.md Outdated Show resolved Hide resolved
usage-events.py Outdated Show resolved Hide resolved
@emilykl
Copy link
Contributor

emilykl commented Oct 11, 2023

@Farkites I just finished a final review over everything except the tests -- looks great!!! 🥇

@emilykl
Copy link
Contributor

emilykl commented Oct 12, 2023

@Farkites I was able to run all the Selenium tests locally! I used a different command to run the tests than what's listed in the CONTRIBUTING.md, I'd recommend getting rid of this bit:

Run the following tests:
python -m unittest tests.test_callbacks
python -m unittest tests.test_interactions
python -m unittest tests.test_usage

and replacing it with

Install ChromeDriver following the instructions [here (Mac)](https://www.kenst.com/2015/03/installing-chromedriver-on-mac-osx/) or [here (Windows)](http://jonathansoma.com/lede/foundations-2018/classes/selenium/selenium-windows-install/). You must install the version of ChromeDriver that matches your Chrome version.

Run the tests using the following command:
pytest --headless

@emilykl
Copy link
Contributor

emilykl commented Oct 12, 2023

@alexcjohnson @Farkites This looks completely ready to me aside from the lingering Percy test issues... I am approving!

  • I see one CI problem where python-3-11 is failing with this error:
Received a 409 error posting to: https://percy.io/api/v1/builds/30612249/finalize.
b'{"errors":[{"status":"conflict","detail":"Can only finalize pending builds"}]}'

It seems like maybe it's failing to finalize because the python-3-11-react-18 has already finalized the build? Do we need to add --nopercyfinalize to one of those commands? Or is there another solution?

  • And then there's the question of the failing image tests. Ultimately it seems to me like we should remove a large number of the image tests since most of them are not testing anything useful on the dash-cytoscape level. For now, it seems like the only new issue added by this PR is the timestamp changing in the parameter outputs -- could we deal with that by telling Percy to ignore that image region?

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

🥳

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Nice work!

@Farkites Farkites merged commit ebbb246 into plotly:master Oct 18, 2023
1 of 2 checks passed
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.

None yet

3 participants