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

[REF] figure: unify chart and image menus #2091

Closed
wants to merge 1 commit into from
Closed

[REF] figure: unify chart and image menus #2091

wants to merge 1 commit into from

Conversation

hokolomopo
Copy link
Contributor

@hokolomopo hokolomopo commented Feb 21, 2023 •
edited
Loading

Description:

Before this commit, the chart and image menus were handled separately in figure_chart.ts and figure_image.ts. This led to a lot of duplicated code since they both do 80% the same thing.

Now the figure menu is handled in figure.ts, and a menuRegistryBuilder was added to figureRegistry.

Odoo task ID : 3199007

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 Feb 21, 2023

Pull request status dashboard.

@hokolomopo hokolomopo force-pushed the master-unify-figure-menu-adrm branch 2 times, most recently from 6c968dc to 78fd612 Compare February 21, 2023 12:24
@anhe-odoo
Copy link
Contributor

LGTM :) I'm just wondering what's the difference between using t-on-contextmenu.prevent.stop="(ev) => !env.isDashboard() and this.onContextMenu(ev)"> vs t-on-contextmenu.prevent.stop="onContextMenu" with an if in the onContextMenu.
You should probably wait for your other task (correction of the context menu position) to be merged to make a rebase ;)

@hokolomopo
Copy link
Contributor Author

LGTM :) I'm just wondering what's the difference between using t-on-contextmenu.prevent.stop="(ev) => !env.isDashboard() and this.onContextMenu(ev)"> vs t-on-contextmenu.prevent.stop="onContextMenu" with an if in the onContextMenu. You should probably wait for your other task (correction of the context menu position) to be merged to make a rebase ;)

Functionnaly it's the same thing, it's just that personnaly I think it's better to have as little code as possible in the xml. And yeah, some rebase will be needed :x

LucasLefevre
LucasLefevre approved these changes Feb 27, 2023
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.

nice :)

src/registries/figure_registry.ts Outdated Show resolved Hide resolved
@hokolomopo hokolomopo force-pushed the master-unify-figure-menu-adrm branch from 78fd612 to 6aabd3e Compare February 27, 2023 13:06
@LucasLefevre LucasLefevre force-pushed the master-unify-figure-menu-adrm branch from 6aabd3e to 0167cef Compare March 9, 2023 16:06
LucasLefevre
LucasLefevre reviewed Mar 9, 2023
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.

rebased, fixed conflicts, and applied comments
Nice ref :)
robodoo r+

src/registries/figure_registry.ts Outdated
});

function getChartMenuRegistry(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function getChartMenuRegistry(
function getChartMenu(

src/registries/figure_registry.ts Outdated
Comment on lines 127 to 128
name: _lt("Delete"),
description: "delete",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete delete 😄
deletedelete

@hokolomopo @LucasLefevre
[REF] figure: unify chart and image menus
b4a5b61 
Before this commit, the chart and image menus were handled separately in
`figure_chart.ts` and `figure_image.ts`. This led to a lot of duplicated code
since they both do 80% the same thing.

Now the figure menu is handled in `figure.ts`, and a `menuRegistryBuilder`
was added to `figureRegistry`.

Odoo task 3199007
@LucasLefevre LucasLefevre force-pushed the master-unify-figure-menu-adrm branch from 0167cef to b4a5b61 Compare March 9, 2023 16:14
@LucasLefevre
Copy link
Collaborator

robodoo r+ prettier foirage

robodoo pushed a commit that referenced this pull request Mar 9, 2023
@hokolomopo @LucasLefevre
[REF] figure: unify chart and image menus
c8dd315 
Before this commit, the chart and image menus were handled separately in
`figure_chart.ts` and `figure_image.ts`. This led to a lot of duplicated code
since they both do 80% the same thing.

Now the figure menu is handled in `figure.ts`, and a `menuRegistryBuilder`
was added to `figureRegistry`.

Odoo task 3199007

closes #2091

Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
@robodoo robodoo temporarily deployed to merge March 9, 2023 17:11 Inactive
@robodoo robodoo closed this Mar 9, 2023
@robodoo robodoo added the 16.2 label Mar 9, 2023
rrahir added a commit that referenced this pull request Mar 15, 2023
@rrahir
[IMP] components: expose FigureComponent to extend chart menus
c8791b4 
recent changes brought by #2091 required to expose another component in
order to extend the chart menus in Odoo.
@rrahir rrahir mentioned this pull request Mar 15, 2023
Closed
14 tasks
rrahir added a commit that referenced this pull request Mar 15, 2023
@rrahir
[IMP] components: expose FigureComponent to extend chart menus
b0871d9 
recent changes brought by #2091 required to expose another component in
order to extend the chart menus in Odoo.
robodoo pushed a commit that referenced this pull request Mar 16, 2023
@rrahir
[IMP] components: expose FigureComponent to extend chart menus
804bae6 
recent changes brought by #2091 required to expose another component in
order to extend the chart menus in Odoo.

closes #2230

Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
@fw-bot fw-bot deleted the master-unify-figure-menu-adrm branch March 23, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucasLefevre LucasLefevre LucasLefevre approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@hokolomopo @robodoo @anhe-odoo @LucasLefevre