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

Update of deck-gl versions to 8.9.34 with fixed CI and drop EOL Python 3.6 and 3.7 #22

Merged
merged 23 commits into from Feb 13, 2024

Conversation

graingert-coef
Copy link
Contributor

@graingert-coef graingert-coef commented Jan 30, 2024

About

this is #20 with my changes that fix CI and an update to deck.gl 8.9.34

Description of changes

Pre-Merge checklist

  • The project was correctly built with npm run build.
  • 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 Deck core contributor.

Reference Issues

Closes #[issue number]

Other comments

@graingert-coef graingert-coef marked this pull request as ready for review January 30, 2024 15:31
@graingert-coef graingert-coef changed the title Update of deck-gl versions to 8.9.32 with fixed CI Update of deck-gl versions to 8.9.34 with fixed CI Jan 30, 2024
@alexcjohnson alexcjohnson mentioned this pull request Jan 30, 2024
6 tasks
@graingert-coef graingert-coef changed the title Update of deck-gl versions to 8.9.34 with fixed CI Update of deck-gl versions to 8.9.34 with fixed CI and drop EOL Python 3.6 and 3.7 Jan 31, 2024
# AUTO GENERATED FILE - DO NOT EDIT

#' @export
''DeckGL <- function(id=NULL, clickEvent=NULL, clickInfo=NULL, data=NULL, disableContext=NULL, dragEndEvent=NULL, dragEndInfo=NULL, dragStartEvent=NULL, dragStartInfo=NULL, enableEvents=NULL, hoverEvent=NULL, hoverInfo=NULL, mapboxKey=NULL, style=NULL, tooltip=NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm ''DeckGL isn't going to work. Somehow that's coming from --r-prefix '' in the build:py_and_r package script. Either we should figure out how to fix this (get it back to just deckGL (somehow this PR is updating that as well?) and delete the files that reference ''DeckGL) or, more likely, since we're not really supporting Dash for R at this point, just remove --r-prefix '' from that command and delete all the R artifacts (R/, inst/deps/, man/, DESCRIPTION, NAMESPACE entirely.

For that matter it'd be great to remove and .gitignore the Python build artifacts as well (all the contents of dash_deck/ except for __init__.py, but that's not a big deal, we can leave that for now. We used to commit build artifacts so people could install directly from the repo, but in most projects we stopped doing that long ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the R and man directories and reran npm run build I think the spurious '' was as a result of @vogt31337 running build:py_and_r on windows. I ran the script from macos and it created the correct files.

I'd like to keep R support and built files in this PR as we're currently deploying our app by installing directly from the repo referencing this branch. I'll delete the built files in a new PR if you'd like

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it - that all makes sense. I know we have various issues with Windows, as none of our core devs use it so we rely on the community to help find and resolve bugs when running there (see eg plotly/dash#2752)

Does that mean you're using this with Dash for R? That's great if so, just curious, we don't always know which of our packages people actually use!

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.

💃 Beautiful!

@alexcjohnson alexcjohnson merged commit fabacf3 into plotly:master Feb 13, 2024
1 check passed
@alexcjohnson alexcjohnson mentioned this pull request Feb 13, 2024
6 tasks
@alexcjohnson
Copy link
Collaborator

@graingert-coef FYI, I was about to make a release (tentatively 0.1.0 but I'm tempted to call it 1.0.0 if we get it fully working) but noticed some errors running demos/usage-events.py:

Screenshot 2024-02-13 at 22 24 28

I didn't try with just this PR so it's possible #26 caused this, though that one should be fairly uncontroversial. My hunch is the structure of certain events got more complex with the new deck.gl version, but it's not clear to me which events are causing this. I also haven't tried any of the other demo apps yet. v0.0.1 does not have this issue.

@graingert-coef graingert-coef deleted the vogt31337/master branch February 14, 2024 10:15
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