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 deprecation warning for mapbox traces and mapbox subplot #7087

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Aug 12, 2024

With the introduction of scattermap, choroplethmap and densitymap as well as map subplots in #7060 this PR adds deprecation warnings to the description of scattermapbox, choroplethmaboxp and densitymapbox as well as mapbox subplots.

@plotly/plotly_js

@archmoj archmoj requested a review from ndrezn August 12, 2024 16:40
@ndrezn ndrezn requested review from emilykl and red-patience August 12, 2024 17:11
Copy link

@red-patience red-patience left a comment

Choose a reason for hiding this comment

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

Okay for me if the style is consistent with other messages in this repo (not super familiar with its conventions)

@gvwilson gvwilson added feature something new P1 needed for current cycle labels Aug 13, 2024
@ndrezn
Copy link
Member

ndrezn commented Aug 13, 2024

Previous deprecation warning example for reference: #5447

@archmoj archmoj requested a review from ndrezn August 13, 2024 15:34
Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

It doesn't look like these deprecations will appear in the console if these traces are used... is that correct? I would expect a console warning

@archmoj
Copy link
Contributor Author

archmoj commented Aug 13, 2024

It doesn't look like these deprecations will appear in the console if these traces are used... is that correct? I would expect a console warning

A warning is now added in the console when using a mapbox subplot.

@archmoj archmoj requested a review from ndrezn August 13, 2024 16:53
@archmoj
Copy link
Contributor Author

archmoj commented Aug 13, 2024

@emilykl Can I have your review on this PR please?

@emilykl
Copy link
Contributor

emilykl commented Aug 13, 2024

@archmoj @ndrezn I would suggest changing the wording of the first sentence to

{trace name} trace is deprecated and will be removed in the next major release.

to give a sense of urgency.

Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

Once we have the link for the migration docs available, we should include that link directly in this deprecation warning.

@archmoj
Copy link
Contributor Author

archmoj commented Aug 26, 2024

@ndrezn Would you please take over this PR?

@ndrezn
Copy link
Member

ndrezn commented Aug 29, 2024

@archmoj , I've added the link where the JS migration guide will live (it is not live yet). I believe that plot-schema.json is autogenerated, I haven't run the build locally before -- could you regenerate this and add to the PR? I think then this PR is good to merge! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants