-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Relates to #3735, adds itemsymbol: 'constant'
legend option, which uses a circle symbol for all legend entries.
#5475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hmm I'm open to advice on the unit test failure, since it seems related to trace ordering, which as far as I can tell has nothing to do with the changes in this PR? |
src/components/legend/attributes.js
Outdated
@@ -77,6 +77,16 @@ module.exports = { | |||
'or remain *constant* independent of the symbol size on the graph.' | |||
].join(' ') | |||
}, | |||
itemsymbol: { | |||
valType: 'enumerated', | |||
values: ['trace', 'constant'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine people may want to have square
or other symbols in future.
So it might be better to use circle
instead of constant
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I just figured I would err on the side of making it work similar to how itemsizing
works. But obviously switching the option to "circle" would be simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe worth noting that if #3735 is ever implemented to expose basically all marker options for the legend, I imagine folks would use that for more extensive customization of the legend symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like circle
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched atributes.js to list the 'circle' option. But in the style.js file, I believe I have set it up to use any valid symbol the user enters for itemsymbol
. Meaning the docs could be expanded to list all valid symbol names as possible values. I'm just not sure where such a list would come from. For now, attributes.js just mentions 'circle'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it to pull in the list of symbols the same way it is done for the scatter trace attributes. I believe that should supply the full list of symbol options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought I'd bump this. I believe I have it set up so it will now accept any symbol name (not just circle) to use as the legend symbol. Anything else left to do before this could be merged in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are failing at the moment.
Could you please sync the master branch of your fork with upstream/master and then merge master into your branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I pulled from the plotly master to my master, then rebased my feature branch the updated master. Something went wonky though, the PR says there are 462 changes. I'll try to figure out what I did wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed the wonky merge. Back to just the 3 changed files.
That test is flaky at the moment. It should pass when I re-run. |
…which uses a circle symbol for all legend entries.
… in theory the user could now specify any valid symbol name.
374ffd8
to
8ba0712
Compare
A small update on timing: our team is working hard on releasing v2.0 of Plotly.js, which we anticipate will happen in early April. This PR would be a good candidate to land in the library in v2.1 or later, so with apologies for the delay, we will likely not be able to give much feedback on this PR for the next few weeks :) |
@brian428 in order for the tests to pass, you need to fetch |
Upon further thought, I think I much prefer the trace by trace approach laid out in #3735 rather than such an absolute legend-level attribute. |
This pull request has been sitting for a while, so I would like to close it as part of our effort to tidy up our public repositories. I've assigned it to myself to keep track of it; I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then. Thanks - @gvwilson |
Closing in regards to a trace by trace approach. |
As the title says, this is similar to the
itemsizing
legend option, but uses a constant legend symbol (circle) instead of using the symbol of the first point in the trace.