Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Fix hover label positioning for bar trace with set `width` by etpinard · Pull Request #1527 · plotly/plotly.js · GitHub
Skip to content

Fix hover label positioning for bar trace with set width - #1527

Merged
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width#1527
alexcjohnson merged 6 commits into
masterfrom
bar-width-hover

Conversation

@etpinard

@etpinardetpinard commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

fixes#1317 - at least the hovermode: 'closest' part, see #1317 (comment) for more details.

TODO:

  • add more tests!

@etpinardetpinard added this to the v1.26.0 milestone Mar 29, 2017
Comment threadsrc/traces/bar/hover.js Outdated
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
dy = function(di, i) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this (providing i as an extra arg) is the most flexible way to handle the issue for future extensibility, but the way that matches better with what we do in scatter - and should be slightly better perf - would be to include the width of each point in di

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah. Good call! Post #1519 this will be easy!

var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

vardata=[{type: 'bar',x: [1],y: [2],width: 0.1},{x:[1.1],y:[1]}];varlayout={xaxis: {range: [-2,2]}};Plotly.newPlot(gd,data,layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

@alexcjohnson

Copy link
Copy Markdown
Collaborator

@etpinard back to you for review. Do the tests I added satisfy the TODO up top?

@etpinard

Copy link
Copy Markdown
ContributorAuthor

@alexcjohnson

Do the tests I added satisfy the TODO up top?

Absolutely.

💃 thanks for wrapping this one up 🎉

@alexcjohnson
alexcjohnson merged commit ce06ea9 into masterApr 5, 2017
@alexcjohnson
alexcjohnson deleted the bar-width-hover branch April 5, 2017 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Manual width breaks hover boxes

2 participants

@etpinard@alexcjohnson