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

Add labelalias to various axes #6481

Merged
merged 17 commits into from Feb 24, 2023
Merged

Add labelalias to various axes #6481

merged 17 commits into from Feb 24, 2023

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Feb 13, 2023

@plotly/plotly_js

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Great to see how compact the code change is! After considering my comment https://github.com/plotly/plotly.js/pull/6481/files#r1115892864 this looks ready to me 🚀

archmoj and others added 2 commits February 23, 2023 11:24
@archmoj
Copy link
Contributor Author

archmoj commented Feb 23, 2023

There might be some limitations related to inherited object keys e.g. __proto__.
@alexcjohnson do you we should test for something like this?

{
    "constructor": "my constructor",
    "hasOwnProperty": "my hasOwnProperty",
    "isPrototypeOf": "my isPrototypeOf",
    "propertyIsEnumerable": "my propertyIsEnumerable",
    "toLocaleString": "my toLocaleString",
    "toString": "my toString",
    "valueOf": "my valueOf",
    "__defineGetter__": "my __defineGetter__",
    "__defineSetter__": "my __defineSetter__",
    "__lookupGetter__": "my __lookupGetter__",
    "__lookupSetter__": "my __lookupSetter__",
    "__proto__": "my __proto__",
    "get __proto__": "my get __proto__",
    "set __proto__": "my set __proto__"
}

Also there may be similar limitations in python dictionaries? cc: @nicolaskruchten

@alexcjohnson
Copy link
Contributor

Oh that's a good point - I don't think any of those built-in keys will be a string unless you do something truly strange... but it wouldn't hurt to tighten up the initial condition if(ax.labelalias) into perhaps:

if(Lib.isPlainObject(ax.labelalias) && ax.labelalias.hasOwnProperty(out.text))

Or maybe even better, move the isPlainObject test into supplyDefaults (ie delete containerOut.labelalias if it's not a plain object) so we don't have to repeat that all the time.

"40": "$40^2$",
"60": "$60^2$",
"80": "$80^2$",
"100": "$100^2$"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if one could possibly add $ in prefix and suffix to render every tick by mathjax?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you would do that, but maybe it would work? Not particularly related to this PR though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have dynamic mathjax labels when you zoom in & out.
Illustrated in c1d127e.

@archmoj
Copy link
Contributor Author

archmoj commented Feb 24, 2023

Oh that's a good point - I don't think any of those built-in keys will be a string unless you do something truly strange... but it wouldn't hurt to tighten up the initial condition if(ax.labelalias) into perhaps:

if(Lib.isPlainObject(ax.labelalias) && ax.labelalias.hasOwnProperty(out.text))

Or maybe even better, move the isPlainObject test into supplyDefaults (ie delete containerOut.labelalias if it's not a plain object) so we don't have to repeat that all the time.

Addressed by 428096e.

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

🌟 💃

@archmoj archmoj merged commit 7427f14 into master Feb 24, 2023
@archmoj archmoj deleted the label-alias branch February 24, 2023 19:20
kMutagene added a commit to plotly/Plotly.NET that referenced this pull request Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants