-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feature/stock-tools-update-perfo #21204
base: master
Are you sure you want to change the base?
Conversation
File size comparisonNo differences found |
Visual test results - No difference found |
Benchmark report - Stockbenchmarks/Stock/Stock-Base.bench.ts
See all
|
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.
Great job!
Apart from the comment below, please try fixing the failing test.
Also, could you add some tests? :)
ts/Stock/StockTools/StockToolbar.ts
Outdated
if (this.options.className !== this.guiClassName) { | ||
if (this.guiClassName) { | ||
this.wrapper.classList.remove(this.guiClassName); | ||
} | ||
if (this.options.className) { | ||
this.wrapper.classList.add(this.options.className); | ||
} | ||
this.guiClassName = this.options.className; | ||
} | ||
|
||
if (this.options.toolbarClassName !== this.toolbarClassName) { | ||
if (this.toolbarClassName) { | ||
this.toolbar.classList.remove(this.toolbarClassName); | ||
} | ||
if (this.options.toolbarClassName) { | ||
this.toolbar.classList.add(this.options.toolbarClassName); | ||
} | ||
this.toolbarClassName = this.options.toolbarClassName; | ||
} | ||
|
||
if ( | ||
!shallowArraysEqual(this.options.buttons, this.buttonList) || | ||
this.isDirty | ||
) { | ||
this.toolbar.innerHTML = AST.emptyHTML; | ||
this.createButtons(); | ||
} | ||
|
||
if (this.options.enabled === false) { | ||
this.destroy(); | ||
} | ||
|
||
if (defined(this.options.visible)) { | ||
this.visible = this.options.visible; | ||
} |
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.
There are a lot of if statements here. Would you mind splitting that into separate methods to improve code readability?
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.
Great work! Just one small comment.
ts/Stock/StockTools/StockToolbar.ts
Outdated
chart.update({ | ||
stockTools: { | ||
gui: { | ||
visible: !self.visible, | ||
placed: true | ||
} | ||
} | ||
}); |
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.
StockTools has it's update method. Maybe we could use that to avoid some unnecessary logic?
PS: there is no placed
property in the API. Should it be there?
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 a few comments, nothing major tbh. Great job overall! 👏
ts/Stock/StockTools/StockToolbar.ts
Outdated
container.appendChild(wrapper); | ||
|
||
this.showhideBtn = createElement('div', { |
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.
Is it showhideBtn
on purpose, or should it be showHideBtn
?
ts/Stock/StockTools/StockToolsGui.ts
Outdated
@@ -144,20 +143,29 @@ function onChartAfterGetContainer( | |||
function onChartBeforeRedraw( | |||
this: Chart | |||
): void { | |||
if (this.stockTools) { | |||
onChartRedraw.call(this); | |||
if (this.stockTools && this.stockTools.guiEnabled) { |
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.
Can we change it to optional chaining?
if (this.stockTools && this.stockTools.guiEnabled) { | |
if (this.stockTools?.guiEnabled) { |
ts/Stock/StockTools/StockToolbar.ts
Outdated
const chart = this.chart, | ||
guiOptions = this.options, | ||
container = chart.container, | ||
navigation = chart.options.navigation, | ||
bindingsClassName = navigation && navigation.bindingsClassName; | ||
bindingsClassName = navigation && navigation.bindingsClassName, |
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.
bindingsClassName = navigation && navigation.bindingsClassName, | |
bindingsClassName = navigation?.bindingsClassName, |
ts/Stock/StockTools/StockToolbar.ts
Outdated
* Remove highcharts-active class from button. | ||
* @private | ||
*/ | ||
public removeButtonActiveClass( |
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'm afraid that 4fd095a removed the usage of this method, and we just have a function, but it's not used anywhere.
So we either need to remove this function, or revert the changes from the commit that removed the usage.
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.
Nice! See my suggestions below
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.
Amazing work!
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.
Merge with master to get rid of failing tests. Other than that it looks good, nice! 🎉
ts/Stock/StockTools/StockToolsGui.ts
Outdated
offsetWidth = listWrapper && ( | ||
this.stockTools.redraw(); | ||
|
||
if (this.stockTools.guiEnabled) { |
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.
Q: What do you think about extracting the whole code below to a function, eg setOffset()
or calculateOffset()
?
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.
All good imo! Please verify that one little comment 👍
* | ||
* @type {boolean} |
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.
Removing that @type
was on purpose? 🤔
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.
Yes #21204 (comment)
Enhance stock tools with optimized update and redraw functionality.