Skip to content

Carson ribbon - #193

Merged
cpsievert merged 13 commits into
masterfrom
carson-ribbon
Mar 24, 2015
Merged

Carson ribbon#193
cpsievert merged 13 commits into
masterfrom
carson-ribbon

Conversation

@cpsievert

@cpsievertcpsievert commented Mar 18, 2015

Copy link
Copy Markdown
Collaborator

This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).

One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.

@cpsievert

cpsievert commented Mar 18, 2015

Copy link
Copy Markdown
CollaboratorAuthor

It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.

@cpsievert

cpsievert commented Mar 19, 2015

Copy link
Copy Markdown
CollaboratorAuthor

Note the new tests --

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html

http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html

The smoother legends still need a bit of work...hopefully I can fix that tomorrow.

@tdhock

tdhock commented Mar 19, 2015

Copy link
Copy Markdown
Contributor

looks good to me. did you add any tests for ribbons and facets?

@cpsievert

cpsievert commented Mar 19, 2015

Copy link
Copy Markdown
CollaboratorAuthor

No, but good idea. Do you think I should put them in a separate test file?

@tdhock

tdhock commented Mar 19, 2015

Copy link
Copy Markdown
Contributor

same file is fine with me.

@cpsievert

cpsievert commented Mar 19, 2015

Copy link
Copy Markdown
CollaboratorAuthor

Here is a test for facetted smooths --

http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html

Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --

http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html

Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?

@cpsievert

cpsievert commented Mar 20, 2015

Copy link
Copy Markdown
CollaboratorAuthor

The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor

http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html

@tdhock

tdhock commented Mar 20, 2015

Copy link
Copy Markdown
Contributor

this plot still has multiple legend entries ... can you add a test for that? and then fix it?

http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html

@cpsievert

cpsievert commented Mar 20, 2015

Copy link
Copy Markdown
CollaboratorAuthor

At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.

@cpsievert

cpsievert commented Mar 20, 2015

Copy link
Copy Markdown
CollaboratorAuthor

OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --

http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html

The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor

http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/

@chriddyp

chriddyp commented Mar 20, 2015

Copy link
Copy Markdown
Member

hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?

@chriddyp

chriddyp commented Mar 20, 2015

Copy link
Copy Markdown
Member

is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image

is the fill a polygon or a trace?

@tdhock

tdhock commented Mar 20, 2015

Copy link
Copy Markdown
Contributor

when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250

for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL

but it is not found. why are logs not getting pushed to the plotly-test-table repos?

it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files

@tdhocktdhock mentioned this pull request Mar 20, 2015
Merged
@cpsievert

cpsievert commented Mar 22, 2015

Copy link
Copy Markdown
CollaboratorAuthor

The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here

@tdhock

tdhock commented Mar 23, 2015

Copy link
Copy Markdown
Contributor

i fixed the bugs and re-built the test table, voilà

http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html

FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).

so this branch looks good to me +1

@cpsievert

cpsievert commented Mar 23, 2015

Copy link
Copy Markdown
CollaboratorAuthor

Nice! Thanks @tdhock!

I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via

http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log

@cpsievert

cpsievert commented Mar 23, 2015

Copy link
Copy Markdown
CollaboratorAuthor

@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)

@tdhock

tdhock commented Mar 23, 2015

Copy link
Copy Markdown
Contributor

for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.

that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).

@cpsievert

cpsievert commented Mar 24, 2015

Copy link
Copy Markdown
CollaboratorAuthor

Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.

@chriddyp

chriddyp commented Mar 24, 2015

Copy link
Copy Markdown
Member

oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).

This looks great to me, let's ship this version, nice work! 💃

@cpsievert

cpsievert commented Mar 24, 2015

Copy link
Copy Markdown
CollaboratorAuthor

Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?

@tdhock

tdhock commented Mar 24, 2015

Copy link
Copy Markdown
Contributor

fine with me +1

cpsievert added a commit that referenced this pull request Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert merged commit a9daa28 into masterMar 24, 2015
@cpsievertcpsievert mentioned this pull request Mar 27, 2015
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp
, '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" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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('^' + ".*" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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('^' + ".*" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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('^' + ".*" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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('^' + ".*" + '
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp

, '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); }
})();
})();
Carson ribbon by cpsievert · Pull Request #193 · plotly/plotly.R · GitHub
Skip to content
Carson ribbon - #193
Merged
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Mar 24, 2015
Merged
Carson ribbon#193
cpsievert merged 13 commits into
masterfrom 
carson-ribbon
Conversation
@cpsievert
@cpsievertcpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

Collaborator
This pull request re-implements ribbon as a basic polygon (in a similar fashion to rect).
One major benefit of doing so is that geom_ribbon() (and consequently geom_smooth()) will respect fill/colour/group aesthetics.
@cpsievert
cpsievert
commented
Mar 18, 2015

Copy link


Copy Markdown

CollaboratorAuthor
It's worth noting that I haven't (yet) added the invisible errorbars (as suggested by Toby) to aid comparison of upper and lower bounds of ribbons upon hover. If there are multiple groups, that could get quite busy, so my suggestion would be to only draw the invisible errorbars when there is 1 group. Does anyone object to that idea? If not, I'll likely implement that in a separate pull request.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Note the new tests --
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/ribbon-fill.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-group.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-colour.html
http://ropensci.github.io/plotly-test-table/tables/39b254657b53c4d5fba34d07c6dde38db64c3ded/smooth-fill.html
The smoother legends still need a bit of work...hopefully I can fix that tomorrow.
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
looks good to me. did you add any tests for ribbons and facets?
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
No, but good idea. Do you think I should put them in a separate test file?
@tdhock
tdhock
commented
Mar 19, 2015

Copy link


Copy Markdown

Contributor
same file is fine with me.
@cpsievert
cpsievert
commented
Mar 19, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Here is a test for facetted smooths --
http://ropensci.github.io/plotly-test-table/tables/aaf302ed8f9fb55b52f90b156425cb9f1124f52e/smooth-facet.html
Unfortunately my attempt to avoid redundant legend entries breaks this plot in a weird way (I'm not sure why) --
http://ropensci.github.io/plotly-test-table/tables/154b7790508a5da192297f191a10e2c94fb337f7/path-line-symbols.html
Does anyone have any other thoughts/opinions on how to best avoid redundant legend entries?
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table looks good to me. Ready to merge when you are @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/index.html
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
this plot still has multiple legend entries ... can you add a test for that? and then fix it?
http://ropensci.github.io/plotly-test-table/tables/2e26cc0e393426fd66c5ae47440c4e1657c343ec/smooth-facet.html
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
At first I was thinking that was the correct behavior since the legend entries have different alpha levels; however, the more I think about it, in most cases, the differences in alpha should be ignored (especially if the entry names are the same). Anyway, I'll go ahead and make sure this becomes one legend.
@cpsievert
cpsievert
commented
Mar 20, 2015

Copy link


Copy Markdown

CollaboratorAuthor
OK, legend entry comparison will now ignore the alpha level, so we get the desired result here --
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-facet.html
The rest of the table looks good to me. Go ahead and merge when ready @tdhock@chriddyp@mkcor
http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
hey sorry just checking in on this. I don't understand why fill/colour/group can't be done with multiple traces and fill?
@chriddyp
chriddyp
commented
Mar 20, 2015

Copy link


Copy Markdown

Member
is it possible to see the json or url for this test? http://ropensci.github.io/plotly-test-table/tables/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.html
image
is the fill a polygon or a trace?
@tdhock
tdhock
commented
Mar 20, 2015

Copy link


Copy Markdown

Contributor
when we make the test table we write a log file for every download attempt https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L250
for your example there is supposed to be this log file http://ropensci.github.io/plotly-test-table/data/a13e92794153452ae16252c6799dba7983462aae/smooth-fill2.log which should contain the plotly URL
but it is not found. why are logs not getting pushed to the plotly-test-table repos?
it is because there should be ./git-add.sh in the push-test-table code. this branch already contains that fix https://github.com/ropensci/plotly/pull/174/files
@tdhocktdhock
mentioned this pull request
Mar 20, 2015
 Merged

@cpsievert
cpsievert
commented
Mar 22, 2015

Copy link


Copy Markdown

CollaboratorAuthor
The test table is slightly broken right now, so let's postpone this until that's fixed. Follow the discussion here
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
i fixed the bugs and re-built the test table, voilà
http://ropensci.github.io/plotly-test-table/tables/ef301b0e7c602b1cb1484f73426ac30dcbf24983/big.html
FYI the rows of the big.html table are now the subset of tests for which there are differences in the md5sum of the png files (between branch and master).
so this branch looks good to me +1
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Nice! Thanks @tdhock!
I merged that pull request which makes logs available, so @chriddyp, you can get the url for that example via
http://ropensci.github.io/plotly-test-table/data/ef301b0e7c602b1cb1484f73426ac30dcbf24983/smooth-fill2.log
@cpsievert
cpsievert
commented
Mar 23, 2015

Copy link


Copy Markdown

CollaboratorAuthor
@chriddyp multiple traces will be generated if the colour/fill aesthetic is specified, but not for group. Do you want multiple traces so that we have more informative tooltips? I think that should be possible...I'm just using the default functionality for polygons (these are actually ribbons -- a special case of a polygon)
@tdhock
tdhock
commented
Mar 23, 2015

Copy link


Copy Markdown

Contributor
for the interactivity it would be nice if the popup was only displayed for x-values with black dots, so then users could compare the observed data value with the mean data value.
that could be achieved if we evaluate the smooth line only on the grid of x-values that are present in the data set (rather than a uniformly spaced grid which I guess is what the code currently does).
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Good idea @tdhock. Unfortunately I'm swamped with other stuff this week. This might have to wait until next week.
@chriddyp
chriddyp
commented
Mar 24, 2015

Copy link


Copy Markdown

Member
oh, nvm, when you guys were saying "basic polygon" I thought you meant Plotly's new basic polygon shapes (https://plot.ly/~etpinard/1647/plotly-shapes/).
This looks great to me, let's ship this version, nice work! 💃
@cpsievert
cpsievert
commented
Mar 24, 2015

Copy link


Copy Markdown

CollaboratorAuthor
Great, I'm keen on merging this since it's such a big bug fix. We can work out the details of comparison mode in another pull request. Are you OK with that @tdhock?
@tdhock
tdhock
commented
Mar 24, 2015

Copy link


Copy Markdown

Contributor
fine with me +1
cpsievert added a commit
that referenced
this pull request
Mar 24, 2015
@cpsievert
Merge pull request#193from ropensci/carson-ribbon
a9daa28
Carson ribbon
@cpsievert
cpsievert
merged commit a9daa28
into
masterMar 24, 2015
@cpsievertcpsievert
mentioned this pull request
Mar 27, 2015

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@cpsievert@tdhock@chriddyp