Skip to content

Fix Plotting Error in other domains besides plot.ly #622

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

Merged
merged 9 commits into from
Dec 1, 2016

Conversation

Kully
Copy link
Contributor

@Kully Kully commented Nov 29, 2016

Fixing #621

@Kully Kully changed the title the fix Fix Plotting Error in other domains besides plot.ly Nov 29, 2016
@Kully
Copy link
Contributor Author

Kully commented Nov 29, 2016

@cldougl


link_domain = plotly_platform_url\
.replace('https://', '')\
.replace('http://', '')
link_text = link_text.replace('plot.ly', link_domain)
link_text = config['linkText'].replace('plot.ly', link_domain)
config['linkText'] = link_text
Copy link
Member

Choose a reason for hiding this comment

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

It's plotting now 🎉
However, the link_text remains: 'Export to plot.ly' while it looks like it should be replaced with the domain

Copy link
Member

Choose a reason for hiding this comment

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

looks good!

@cldougl
Copy link
Member

cldougl commented Nov 29, 2016

cool! want to take one more 👀 at replacing the link text @Kully
Can you also

  • bump version
  • update changelog (you may have to pull my changes)
  • add a test

as suggested here: #621 (comment)

@Kully
Copy link
Contributor Author

Kully commented Nov 29, 2016

@cldougl

@Kully
Copy link
Contributor Author

Kully commented Nov 30, 2016

@cldougl

@cldougl
Copy link
Member

cldougl commented Dec 1, 2016

@Kully great!! can you update the changelog then looks good to 💃 !

@Kully
Copy link
Contributor Author

Kully commented Dec 1, 2016

@cldougl Let me know if the CHANGELOG looks okay. I know you had to change my wording on other sections :)

@@ -2,7 +2,9 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
## [1.12.11]
Copy link
Member

Choose a reason for hiding this comment

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

@Kully you can add today's date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!

## [Unreleased]
## [1.12.11]
### Fixed
- Offline plotting now works in other domains besides `plot.ly`. In addition, the `link text` in the bottom right corner of the offline plots properly display `Export to [Domain Name]` for the given Domain name of the server.
Copy link
Member

Choose a reason for hiding this comment

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

ahh this may seem nitpicky but offline plotting doesn't work 'in' a given domain (cause it's offline), it was just an issue with the link text not getting assigned at the right time. This could just be:
The link text in the bottom right corner of offline plots now properly displays Export to [Domain Name] for the given domain set in the user's .config file.

@Kully Kully merged commit ca9dd35 into master Dec 1, 2016
@Kully Kully deleted the fix-plot-other-domain branch December 1, 2016 16:23
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.

2 participants