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

Exhibition widget and a more sane repr for closed UniverseWidget #200

Closed
wants to merge 93 commits into from

Conversation

tjduigna
Copy link
Contributor

@tjduigna tjduigna commented Jul 14, 2020

Hey I saw you assign #192 to me, thanks for reminding me about this. There are 3 open issues with this MR:

  1. The new widget has a kludge for sizing the renderer. The method still uses offsetHeight rather than canvas height but the growing/shrinking widget is actually related to the element offsetHeight instability during kernel reset. Instead, it makes an explicit check whether the widget is connected to the kernel before attempting to resize.
  2. The selenium script throws a timeout exception in the travis build. Not so much a workaround, more of a punt. The test script will handle this exception. It is still a useful development test script and brings us much closer to CI testing for widgets.
  3. The CI pipeline does not work for both linux and macos right now. Parametrizing both chrome and firefox driver installation on both mac and linux is complete. This at least makes it easier to resolve 2 in the future.

Copy link
Member

@avmarchenko avmarchenko left a comment

Choose a reason for hiding this comment

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

Added a few comments/questions. The exhibition_widget is a very nice addition (both for advertising and for ad-hoc testing). I'm thinking there are two pieces to automate those tests: first need to use selenium or something to automate browser actions and second need to include some kind of successful message when all promises are fulfilled (and rejection messages when not fulfilled). Is that about right or not quite?

js/src/scene.ts Outdated Show resolved Hide resolved
js/src/scene.ts Outdated Show resolved Hide resolved
@tjduigna
Copy link
Contributor Author

I like the idea of sending a message back to the kernel on promise fulfillment. And yes presumably we can set up a selenium test to run in CI. I will see what it will take to get some of that going as part of this PR.

Copy link
Member

@avmarchenko avmarchenko left a comment

Choose a reason for hiding this comment

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

Thank you for implementing automatic testing. Thank you for the massive updates to the integration with three.js. I'm a big fan of the use of TS as well. The changes are well thought out and now that we have some testing (plus the exhibition widget) it's pretty easy to make sure future changes don't break things.

js/src/scene.ts Show resolved Hide resolved
js/src/scene.ts Show resolved Hide resolved
install-chromedriver.sh Show resolved Hide resolved
@avmarchenko avmarchenko self-requested a review December 23, 2020 19:20
@tjduigna tjduigna added the Visualizations Feature requests related to visualization functionality label Nov 3, 2021
@avmarchenko
Copy link
Member

Closing as stale - will reopen as appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Visualizations Feature requests related to visualization functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automated npm publish broken
3 participants