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.

Sign up for GitHub

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

Jump to bottom

[FIX] Charts: fix incoherent panel state #2929

Closed
wants to merge 1 commit into from
Closed

[FIX] Charts: fix incoherent panel state #2929

wants to merge 1 commit into from

Conversation

anhe-odoo
Copy link
Contributor

@anhe-odoo anhe-odoo commented Sep 25, 2023 •
edited

Description:

This task aims to address some issues in the side panel of a chart:

  1. When having only one point in a dataseries and selecting "First
    row as header", the point disappear from the chart (legit), but
    we don't have the possibility to uncheck this checkbox.
  2. When having a normal dataset and another dataset containing one
    point, checking the "... as header" checkbox works has expected, but
    unchecking it don't make the 'one-point' dataset come back

Related Task:

Task: 3380568

Related Task(s):

Review Checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_lt("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Sep 25, 2023

Pull request status dashboard.

@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch 2 times, most recently from 8311491 to ad9293f Compare September 26, 2023 10:05
@anhe-odoo anhe-odoo changed the title [FIX] Charts: fix incoherent pannel state [FIX] Charts: fix incoherent panel state Sep 28, 2023
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from ad9293f to 807ac9b Compare October 2, 2023 12:49
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 807ac9b to 914e227 Compare October 11, 2023 07:53
hokolomopo
hokolomopo reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

Seems to work 👍 I'm not gonna comment on the SelectionInput changes, as long as it works I don't care, we should burn this whole things to the ground and code something better.

Needs a rebase :)

src/components/side_panel/chart/gauge_chart_panel/gauge_chart_config_panel.ts Outdated Show resolved Hide resolved
src/helpers/figures/charts/bar_chart.ts Outdated Show resolved Hide resolved
src/helpers/figures/charts/bar_chart.ts Outdated Show resolved Hide resolved
src/helpers/figures/charts/bar_chart.ts Outdated Show resolved Hide resolved
tests/components/charts.test.ts Outdated Show resolved Hide resolved
@rrahir rrahir force-pushed the master-chart-incoherent_panel_state-anhe branch from 914e227 to 30268d3 Compare November 1, 2023 17:54
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 30268d3 to 5444acd Compare November 2, 2023 11:44
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch 2 times, most recently from b47bfb5 to 32d431e Compare November 22, 2023 15:08
@anhe-odoo anhe-odoo changed the base branch from master to 16.0 November 22, 2023 15:09
hokolomopo
hokolomopo reviewed Nov 24, 2023
View reviewed changes
Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

👍

src/helpers/charts/abstract_chart.ts Outdated Show resolved Hide resolved
src/helpers/charts/bar_chart.ts Outdated Show resolved Hide resolved
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 32d431e to 3b81f59 Compare November 24, 2023 14:46
LucasLefevre
LucasLefevre reviewed Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment •
edited

Choose a reason for hiding this comment

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

is it possible to split the commit in 2 commits? one for each issue described. Or is it chiant ?
It's easier to review when each change is isolated. It's also easier for the future when someone will try to understand it.

@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 3b81f59 to 3a608ea Compare December 6, 2023 10:13
@anhe-odoo
Copy link
Contributor Author

@LucasLefevre : the fix is the same for both issues, but the code has been simplified as discussed together so it should be easier to review :)

LucasLefevre
LucasLefevre reviewed Jan 15, 2024
View reviewed changes
src/helpers/charts/bar_chart.ts Outdated
@@ -142,7 +147,7 @@ export class BarChart extends AbstractChart {
}

getDefinition(): BarChartDefinition {
return this.getDefinitionWithSpecificDataSets(this.dataSets, this.labelRange);
return this.getDefinitionWithSpecificDataSets(this.fullDataSets, this.labelRange);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the issue/fix anymore :/
in a DataSet, dataRange includes labelRange...so it should end up being the same dataset string in the definition 🤔
I'll wait until you come back

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some data is lost inside createDataSets when there's a single cell range and it includes (only) the title => the dataset is dropped (it should not)

Let's try reverting the fix below

see #2442

@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 3a608ea to 16fc7a0 Compare January 22, 2024 12:32
LucasLefevre
LucasLefevre reviewed Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

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

only some cleaning I think 😉

src/helpers/charts/chart_common.ts Outdated Show resolved Hide resolved
src/helpers/charts/chart_common.ts
@@ -182,11 +182,6 @@ export function createDataSets(
)
);
}
} else if (zone.left === zone.right && zone.top === zone.bottom) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the end, all the diff in this PR except this one is useless right ? (and the tests)
Could you revert the useless diff ?
Even if some changes are clearer, it's not worth it here IMO (especially for a bug fix in stable). Useless changes makes the review more difficult, forward-ports (and future forward-ports) more likely to conflict.

As a general rule of thumb, keep the diff minimal for bug fixes in stable 😉 Refactorings are better in master and in different commits (not burried in a bug fix or a new feature)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I though the other diff with the this.dataSetsHaveTitle things were necessary, but not with this change :)

@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 16fc7a0 to 7795ac2 Compare January 29, 2024 07:10
@anhe-odoo
[FIX] chart: fix incoherent side_pannel state
8331d6e 
Task Description

This task aims to adress some issues in the side pannel of a chart:
1. When having only one point in a dataseries and selecting "First
row as header", the point dissapear from the chart (legit), but
we don't have the possibility to uncheck this checkbox.
2. When having a normal dataset and another dataset containing one
point, checking the "... as header" checkbox works has expected, but
unchecking it don't make the 'one-point' dataset come back

Related Task:

Task: 3380568
@anhe-odoo anhe-odoo force-pushed the master-chart-incoherent_panel_state-anhe branch from 7795ac2 to 8331d6e Compare January 29, 2024 07:11
LucasLefevre
LucasLefevre reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

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

robodoo r+
Thanks :)

robodoo pushed a commit that referenced this pull request Jan 29, 2024
@anhe-odoo
[FIX] chart: fix incoherent side_pannel state
ebb77e2 
Task Description

This task aims to adress some issues in the side pannel of a chart:
1. When having only one point in a dataseries and selecting "First
row as header", the point dissapear from the chart (legit), but
we don't have the possibility to uncheck this checkbox.
2. When having a normal dataset and another dataset containing one
point, checking the "... as header" checkbox works has expected, but
unchecking it don't make the 'one-point' dataset come back

Related Task:

closes #2929

Task: 3380568
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
@robodoo robodoo closed this Jan 29, 2024
This was referenced Jan 29, 2024
Closed
Closed
Closed
@rrahir rrahir deleted the master-chart-incoherent_panel_state-anhe branch February 1, 2024 07:43
@fw-bot fw-bot mentioned this pull request Feb 1, 2024
Closed
14 tasks
@fw-bot
Copy link
Collaborator

fw-bot commented Feb 2, 2024

@anhe-odoo @LucasLefevre this pull request has forward-port PRs awaiting action (not merged or closed):

This was referenced Feb 2, 2024
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucasLefevre LucasLefevre LucasLefevre left review comments

@hokolomopo hokolomopo hokolomopo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@anhe-odoo @robodoo @fw-bot @LucasLefevre @hokolomopo