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

Trace meta text #3865

Merged
merged 7 commits into from
May 21, 2019
Merged

Trace meta text #3865

merged 7 commits into from
May 21, 2019

Conversation

etpinard
Copy link
Contributor

a follow-up #3439 and #3548 - where we:

  • allow meta to be set as an {} (not just as an []) and
  • allow users to set meta info in each trace container.

To make it easier for users to grab trace meta info within the trace where meta is set, we proposed the following precedence rules:

Given

Plotly.newPlot(gd, [{
  y: [1, 2, 1],
  meta: { yname: "dollars" }
},{
  y: [2, 3, 4],
  marker: {color: [1, 2, 3]},
  meta: { yname: "temperature" }
}], {
  meta: [
    'Company B'
  ]
})

Setting

gd.data[0].name = '%{meta.yname}' // => 'dollars'
gd.data[1].marker.colorbar.title.text = '%{meta.yname}' // => 'temperature'

but %{meta.yname} gives a blank string in layout fields (e.g. layout.xaxis.title.text).

One could still use layout.meta within the traces as

gd.data[0].name = '%{meta[0]}' // =>  'Company B' (as key '0' is not overridden by trace.meta)

// or 
gd.data[0].name = '%{layout.meta[0]}' // => 'Company B'

Moreover, one can target trace meta info in layout fields as

gd.layout.yaxis.title.text = '%{data[0].meta.yname} vs. %{data[1].meta.yname}'

I hope @plotly/plotly_js will like it.

@nicolaskruchten
Copy link
Member

This looks good to me. I had forgotten that in 1.47, an un-prefixed trace.name = '%{meta[0]}' will resolve to layout.meta[0]. I think it's perhaps not too late to break that and require the layout. prefix always?

Either way, this PR looks like we need, thanks!

@etpinard
Copy link
Contributor Author

etpinard commented May 15, 2019

This looks good to me. I had forgotten that in 1.47, an un-prefixed trace.name = '%{meta[0]}' will resolve to layout.meta[0].

That will still work. The meta ref for traces is extendFlat({}, layout.meta, trace.meta).

So,

trace = {
  meta: {yname: 'Y'},
  name: '%{meta.yname} - %{meta[0]}'
}
layout = {
  meta: ['Company B']
}

will print Y - Company B

@alexcjohnson
Copy link
Collaborator

The meta ref for trace is extendFlat({}, layout.meta, trace.meta).

I guess if there isn't a trace.meta it makes sense to fall back on layout.meta - especially for backward compatibility for anyone currently using layout.meta in trace contexts. But merging the two seems a bit confusing, esp. since we're allowing - expecting even - that one will be an array and the other an object. I'd vote to just use trace.meta || layout.meta.

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Very nice implementation. Thanks @etpinard.
Please find my question below. Also two tests are failed on the CI.

@@ -97,8 +99,10 @@ function draw(gd, titleClass, options) {
if(!editable) txt = '';
}

if(fullLayout.meta) {
txt = Lib.templateString(txt, {meta: fullLayout.meta});
if(options._meta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we need to bypass [ ] or { } here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. Lib.termplateString handles those cases correctly.

@nicolaskruchten
Copy link
Member

The downside of falling back is that if today I have

trace = {
  name: '%{meta[0]}'
}
layout = {
  meta: ['Company B']
}

And then I set trace.meta = {} it will break in a way which is a bit confusing perhaps.

It's not a huge concern either way. I think there are probably zero users of layout.meta PLUS trace.anything = '%{meta[ ... ]}' in the wild today (including Chart Studio) so I'd personally be OK with the case above not working as of 1.48.

... during trace templateString calls.
@nicolaskruchten
Copy link
Member

"interesting" concern...

data[0] = {
   meta: {x: 'hi'},
   name: '%{meta.x}',
   hovertemplate: '%{name}'
}

What will show up in the hover? What if I don't set hovertemplate? I assume the trace name shows up in the extra section as hi?

@etpinard
Copy link
Contributor Author

etpinard commented May 17, 2019

Plotly.newPlot(gd, [{
   y: [1], hoverinfo: 'all',
   meta: {x: 'hi'},
   name: '%{meta.x}',
   hovertemplate: '%{data.name}'
}])

currently shows
Peek 2019-05-17 11-37

and w/o hovertemplate:

Plotly.newPlot(gd, [{
   y: [1], hoverinfo: 'all',
   meta: {x: 'hi'},
   name: '%{meta.x}'
}])

we get:

Peek 2019-05-17 11-38

@nicolaskruchten
Copy link
Member

Yeah, that's what I figured. OK for now, and probably forever, given the crazy recursion possibilities. Wouldn't want to make our hovertemplate system Turing-complete.

@etpinard etpinard mentioned this pull request May 20, 2019
3 tasks
@etpinard
Copy link
Contributor Author

etpinard commented May 21, 2019

Is everyone happy with the behaviour here?

If so, @antoinerg would you mind reviewing this at some point today? Thanks!

@antoinerg antoinerg self-assigned this May 21, 2019
@nicolaskruchten
Copy link
Member

I’m happy!

@antoinerg
Copy link
Contributor

antoinerg commented May 21, 2019

Is everyone happy with the behaviour here?

I was really happy until I realized the following test fails:

            Plotly.update(gd, {
                meta: {pizza: 'is yummy'},
                hovertemplate: 'TRACE -- %{meta.yname}<extra>%{meta.xname}</extra>'
            }, {
                meta: {yname: 'Yy', xname: 'Xx'}
            })
            .then(function() {
                Fx.hover('graph', evt, 'xy');

                assertHoverLabelContent({
                    nums: 'TRACE -- Yy',
                    name: 'Xx',
                    axis: '0.388'
                });
            })

If meta is defined at the trace-level and the requested key is missing, we don't make the effort of looking into the layout.meta object to see if it exists there. As a user, I kinda expected it to do so but maybe it's a bad idea? 🤔

Anyway, I think my suggested behavior could be implemented by making Lib.templateString a variadic function (similar to Lib.hovertemplateString) and changing Lib.templateString(d.name, d.trace._meta) to Lib.templateString(d.name, d.trace._meta, d.trace._meta.layout).

Let me know what you think @nicolaskruchten @plotly/plotly_js

@nicolaskruchten
Copy link
Member

I really think we should drop access to layout.meta via meta in trace.any_template. This feature was requested by me and even we never used it in prod in Chart Studio because what we really needed was this PR. I very strongly doubt anyone is using it in the wild: it wasn't documented anywhere.

@antoinerg
Copy link
Contributor

I really think we should drop access to layout.meta via meta in trace.any_template.

Oh, I see @nicolaskruchten! I didn't realize this was a conscious decision. @alexcjohnson can you confirm that you are OK with this? If so, then the PR would be good to go since its new behavior is well 🔒 down by the mock.

@alexcjohnson
Copy link
Collaborator

I really think we should drop access to layout.meta via meta in trace.any_template.

Confirmed, I'm happy with this.

@antoinerg
Copy link
Contributor

💃 💃
💃 💃

@etpinard etpinard merged commit 66b2f40 into master May 21, 2019
@etpinard etpinard deleted the trace-meta branch May 21, 2019 17:26
@nicolaskruchten
Copy link
Member

Confirmed, I'm happy with this.

Well, that's not what we went with here, right? here it uses layout.meta as meta if trace.meta isn't present. I was thinking we wouldn't even do that. But I'm not hung up on it :)

@antoinerg
Copy link
Contributor

here it uses layout.meta as meta if trace.meta isn't present.

Yes it does @nicolaskruchten.

I was thinking we wouldn't even do that. But I'm not hung up on it :)

Ok good. I'll let @etpinard decide on this one.

@nicolaskruchten
Copy link
Member

well it's merged, so @etpinard already decided. Works for me, just wanted to make sure we're all on the same page. No further work or discussion required on this today :)

@etpinard etpinard mentioned this pull request Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants