[Bug] fix TSVB y-axis #2079
[Bug] fix TSVB y-axis #2079
Conversation
Signed-off-by: AbhishekReddy1127 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2079 +/- ##
=======================================
Coverage 67.23% 67.23%
=======================================
Files 3100 3100
Lines 59564 59564
Branches 9062 9062
=======================================
Hits 40047 40047
Misses 17331 17331
Partials 2186 2186 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
@AbhishekReddy1127 Great find!
Usually the hard work in a bug fix like this is the sleuthing that it takes to understand the what caused the buggy behavior (and, in a regression like this, when/what introduced it).
Can you add an explanation of the fix to the issue description (and commit body)? That will help future devs understand why/how this fixed the bug.
In addition, for a UI issue, it's useful to have a screenshot or animation in the description that shows that it fixes the bug. Are there any tests you have found that should be updated to prevent a similar regression in the future?
+1, yes thanks @AbhishekReddy1127 for getting this. But will help will why removing this was the bug and also ensuring we don't have any regressions. |
| @@ -212,7 +212,6 @@ export class TimeseriesVisualization extends Component { | |||
| seriesDataRow.groupId = groupId; | |||
| seriesDataRow.yScaleType = yScaleType; | |||
| seriesDataRow.hideInLegend = Boolean(seriesGroup.hide_in_legend); | |||
| seriesDataRow.useDefaultGroupDomain = !isCustomDomain; | |||
There was a problem hiding this comment.
Could we post the behavior, for before / after change to ensure we are getting expected behavior ?
There was a problem hiding this comment.
@kavilla @mihirsoni @joshuarrrr @ananzh
This is how the Y-axis was looking due to the bug.
But in my investigation I came to know that, this was happening because of a prop useDefaultGroupDomain. Actually this prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So, I felt useDefaultGroupDomain may not be the right prop for TSVB.
Either we can remove the line 215 as I did above or by setting the useDefaultGroupDomain to false is also solving this issue.
After removing the line, the output is looking like:
or, either setting the useDefaultGroupDomain to false
As,@joshuarrrr asked about the tests, i could think of like to verify whether the useDefaultGroupDomain is set to false so that TSVB will work as expected.
This is what I have with me until now. Please correct me if i am wrong anywhere.
|
From what I see in the code: where ... which indicates that the global default |
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4)
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4)
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4) Co-authored-by: Abhishek Reddy <[email protected]>
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4) Co-authored-by: Abhishek Reddy <[email protected]>
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4)
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4)
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4) Co-authored-by: Abhishek Reddy <[email protected]>
Issue: #1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4) Co-authored-by: Abhishek Reddy <[email protected]>
…ch-project#2107) Issue: opensearch-project#1873 This was happening because of a prop useDefaultGroupDomain. This prop was introduced into the elastic charts to allow clustering for the multiple series which will share a common Y domain i.e for example the stacked and non- stacked bars will have the same Y common domain/axis. As per the elastic/charts whenever the useDefaultGroupDomain is set to true, it will force to group the series/visualiations groupId's to a default group domain. But, TSVB is not a shared Y domain. So the prop is unneeded. Further insight: From the code: ``` const groupId = hasSeparateAxis || isStackedWithinSeries ? seriesGroup.id : mainAxisGroupId; ``` where ``` const mainAxisGroupId = yAxisIdGenerator('main_group'); ``` ... which indicates that the global default `groupId` is not used by TSVB. Hence, TSVB should have not been using `useDefaultGroupDomain` ever. Signed-off-by: AbhishekReddy1127 <[email protected]> (cherry picked from commit 55181d4) Co-authored-by: Abhishek Reddy <[email protected]>
Signed-off-by: AbhishekReddy1127 [email protected]
Description
Fixed the TSVB y-axis.
Issues Resolved
#1873
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr