Fix: update layout titles for Plotly.js v3.0.0 compatibility (closes …#2827
Fix: update layout titles for Plotly.js v3.0.0 compatibility (closes …#2827Avinash123643 wants to merge 4 commits into
Conversation
zhaoshanren8808-ship-it
left a comment
There was a problem hiding this comment.
Code Review: Plotly v3 Compatibility Fix
感谢这个 PR!修复 Plotly v3 标题格式兼容性是个重要的更新。以下是一些需要改进的地方:
🔴 必须修复
1. 注释掉的代码未删除 (chart_config_builder.py)
#_layout = attributes.get("layout") or {}这行注释掉的代码应该删除。
2. 文件末尾缺少换行符 (Chart.tsx)
文件末尾的 export default Chart; 后缺少空行。
🟡 代码风格问题 (PEP 8)
3. 注释格式不规范
#Fix yaxis Title format ← 应为: # Fix yaxis Title format
#Fix xaxis Title format ← 应为: # Fix xaxis Title format
#Fix chart title format ← 应为: # Fix chart title format4. 类型注解和赋值操作符缺少空格
_axis_layout: t.Dict[str,t.any]={}
# 应改为:
_axis_layout: t.Dict[str, t.any] = {}
_yaxis_val=_layout.get("yaxis", {})
# 应改为:
_yaxis_val = _layout.get("yaxis", {})
_xaxis_val=_layout.get("xaxis", {})
# 应改为:
_xaxis_val = _layout.get("xaxis", {})
_chart_title=_layout.get("title")
# 应改为:
_chart_title = _layout.get("title")
_xaxis_val["title"]={"text": _xaxis_title}
# 应改为:
_xaxis_val["title"] = {"text": _xaxis_title}📝 测试文件 (Chart.spec.tsx)
5. 注释掉的测试代码
//it("handle figure layout title in Plotly v3 object format", async () => {注释掉的测试代码应该删除或实现。
总结:核心逻辑正确,toPlotlyTitle helper 设计良好。主要是代码风格问题需要修复。
建议:修复以上问题后,可以合并。
|
Thnx for review! I have addressed all the issues: Please review again when you get a chance. thnx |
|
Thanks a lot @Avinash123643 for this fine work, and ease of use from the programmer's stand point, regardless of what Plotly version Taipy relies on (which is the point of making charts available without knowing what implements it). I'm in the process of reviewing your code, but I wanted to warn that I am going to request not to bother with the back-end side of things: we don't really need to modify the layout dictionary before it gets send to the front-end, as long as the JS code takes care (as you have implemented) of converting the legacy syntax to the new. I don't think we gain anything by doing this (except being able to test - as you do - the situation and fix it by building the appropriate dict). I hope you understand my proposal:
Please let me know if you don't agree and why, before I simplify the PR content. |
|
And thanks a lot @zhaoshanren8808-ship-it for your review. |
…#2472)
What type of PR is this? (Check all that apply)
Description
Related Tickets & Documents
How to reproduce the issue
Backporting
This change should be backported to:
Checklist
We encourage keeping the code coverage percentage at 80% or above.
If not, explain why:
If not, explain why:
If not, explain why:
If not, explain why: