Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting legend names when multiple series values are used in an Indicator #382 #385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zlpatel
Copy link
Contributor

@zlpatel zlpatel commented Jun 16, 2021

Fixes #382

@@ -105,6 +105,10 @@ def I(self, # noqa: E741, E743
If `scatter` is `True`, the plotted indicator marker will be a
circle instead of a connected line segment (default).

`legends` can be list or array of string values to represent
legends on your indicator chart. By default it's set to None,
and `name` is used as legends.
Copy link
Owner

Choose a reason for hiding this comment

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

Why don't we simply reuse name= to accept a list of name values?

Copy link
Contributor Author

@zlpatel zlpatel Jun 23, 2021

Choose a reason for hiding this comment

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

@kernc I didn't use it because below line in _plotting.py expects name to be a single value and is being used while storing the source that is being passed in to customJS callback.

source_name = f'{legend_label}_{i}_{j}'

I think we can't treat it the same way as we do with color, as the scope of color is limited to plotting the indicator. But name is used as a key in other places to determine the auto scale Y axis range. Let me know, if you think otherwise. We can discuss further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kernc did you get a chance to look at this?

Copy link

Choose a reason for hiding this comment

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

I am pretty sure, it should be possible to reuse name=, since color= is used in the same fashion. For instance here is an example how to set multiple colors.

Having two arguments to set one thing is very confusing from the API perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivaigult Can you try and let me know. My first attempt was to pass array to name but it didn't work. I already mentioned my reasoning in previous comment. If you can make it work with name that would be great :)

Copy link

Choose a reason for hiding this comment

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

@zlpatel I uploaded my attempt in #980 🙂

@kernc kernc force-pushed the master branch 3 times, most recently from 7fd493d to e7981c7 Compare December 13, 2021 01:18
Copy link

@aradng aradng left a comment

Choose a reason for hiding this comment

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

can we get a merge this been sitting here for 3 months

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting legend names when multiple series values are used in an Indicator
5 participants