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

Feature: Plot layout images #525

Merged
merged 20 commits into from
May 12, 2016
Merged

Feature: Plot layout images #525

merged 20 commits into from
May 12, 2016

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented May 11, 2016

Continuing from before - fulfilling #496:

This PR adds the ability to add arbitrary images loaded from a url to a plot. Plot layout images follow a similar interface to shapes, but use a coordinate position with sizex and sizey rather than x0, x1, y0, y1. Additionally, there are three sizing modes - contain (the default), fill, and stretch.

The intended use of this feature is mainly for logos and non-data-specific images - for image data that is tied directly to data (e.g. satellite imagery) we should/will add a new trace type to accommodate for this and enable features such as toggling from the legend and ordering.

That said, images can be added to the plot area and be forced to keep their binding to the underlying cartesian plane by setting the xref and yref to their respective axes and setting the sizing property to stretch.

Since the initial PR, a few things have been changed:

  • Image loading adds a promise to gd._promises
  • A key function has been added for relayout updates to images
  • width/height have been changed to sizex/sizey
  • More tests
  • A wicked-awesome baseline image 🎉

@@ -182,6 +182,7 @@ module.exports = {
'legend': 'Legend',
'annotations': 'Annotations',
'shapes': 'Shapes',
'images': 'Images',
Copy link
Contributor

Choose a reason for hiding this comment

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

@mdtusz can you also add images to this test list ?

@etpinard
Copy link
Contributor

etpinard commented May 12, 2016

@mdtusz Looking great. Here are my TODOs:

  • Feature: Plot layout images #525 (comment)
  • I think you got sizex and sizey mixed up in the attributes
  • make sure that we are not vulnerable to XSS here
  • I think the image draw module is missing some logic to handle xanchor and yanchor to be on-par with annotations (ref).
  • add a few test cases for Lib.toObject in lib_test.js
  • Looks like images are always drawn above shapes. Not sure what's best. @mdtusz what's your rationale on this topic?
  • we should add one data:image image to the test json mock

@mdtusz
Copy link
Contributor Author

mdtusz commented May 12, 2016

I'm not too sure how best to deal with shapes/images layering. I'd prefer if we kept it absolute so there's less convoluted logic on the layering though (and also, it would get pretty messy trying to define the order they are drawn). Images are on top right now purely because of the order their <g> containers are added though, so I'm happy to swap them - might make more sense so people can draw things atop the images for whatever reason.

@etpinard
Copy link
Contributor

I'd prefer if we kept it absolute so there's less convoluted logic on the layering though

good call.

might make more sense so people can draw things atop the images for whatever reason.

+1. Let's go with that.

* // returns { nested: { test: [null, null, { path: 'value' }]}
*
* @param {string} path to nested value
* @param {*} any value to be set
*
* @return {Object} the constructed object with a full nested path
*/
lib.toObject = function(path, value) {
lib.objectFromPath = function(path, value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

an even more verbose plotly-esque name would be Lib.objectFromAttributeString or Lib.objectFromAstr.

@mdtusz I'll leave it up to you to decide what's best.

@etpinard
Copy link
Contributor

💃

@mdtusz mdtusz merged commit 5940dd1 into master May 12, 2016
@mdtusz mdtusz deleted the plot-images branch May 12, 2016 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants