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.

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

[FIX] Selection: Altering selection should scroll the viewport #1775

Closed

Conversation

rrahir
Copy link
Collaborator

@rrahir rrahir commented Oct 31, 2022

Description:

description of this task, what is implemented and why it is implemented that way.

Odoo task ID : 3050101

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

@rrahir rrahir changed the base branch from master to saas-15.3 October 31, 2022 11:22
@robodoo
Copy link
Collaborator

robodoo commented Oct 31, 2022

@rrahir rrahir changed the title Saas 15.3 fix selection again no days off rar [FIX] Selection: Altering selection should scroll the viewport Oct 31, 2022
@rrahir rrahir force-pushed the saas-15.3-fix-selection-again-no-days-off-rar branch from c143b27 to c6ac43b Compare October 31, 2022 14:36
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 details

src/plugins/ui/autofill.ts Outdated Show resolved Hide resolved
this.autofillZone = undefined;
this.selection.resizeAnchorZone(this.direction, this.steps);
Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, looks like what it should have been from the start

@@ -142,7 +142,10 @@ export class SelectionStreamProcessor
/**
* Set the selection to one of the cells adjacent to the current anchor cell.
*/
moveAnchorCell(direction: SelectionDirection, step: SelectionStep = "one"): DispatchResult {
moveAnchorCell(direction: SelectionDirection, step: SelectionStep = 1): DispatchResult {
if (step !== "end" && step <= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it work when step=0 ? Probably yes, but it's worth a test

tests/components/autofill.test.ts Show resolved Hide resolved
@@ -487,7 +487,7 @@ export function mergeOverlappingZones(zones: Zone[]) {
export function findCellInNewZone(
oldZone: Zone,
currentZone: Zone,
viewport: Viewport
viewport?: Viewport
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe update the docstring to help understanding what impact the viewport has

@rrahir rrahir force-pushed the saas-15.3-fix-selection-again-no-days-off-rar branch 4 times, most recently from 49f77af to f22726e Compare November 9, 2022 07:29
Commit b437f44 introduced an error. while fixing the viewport
scrolling while in autofill (it shouldn't happen) it also prevent an
change of selection (not an extension) to properly scroll the viewport.

The problem ultimately lies in the fact that the autofill was using the
selectionProcessor to override its selection instead of extending it.

In order to fix this, it was necessary to alter the API of the
selection processor to allow more granularity in the steps to extend a
zone selection: i.e. instead of a step of 1 or jump to the end of a zone,
we need to support all POSITIVE values of steps.

Task 3050101
@LucasLefevre LucasLefevre force-pushed the saas-15.3-fix-selection-again-no-days-off-rar branch from f22726e to c39291e Compare November 21, 2022 16:16
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+

src/helpers/zones.ts Outdated Show resolved Hide resolved
robodoo pushed a commit that referenced this pull request Nov 21, 2022
Commit b437f44 introduced an error. while fixing the viewport
scrolling while in autofill (it shouldn't happen) it also prevent an
change of selection (not an extension) to properly scroll the viewport.

The problem ultimately lies in the fact that the autofill was using the
selectionProcessor to override its selection instead of extending it.

In order to fix this, it was necessary to alter the API of the
selection processor to allow more granularity in the steps to extend a
zone selection: i.e. instead of a step of 1 or jump to the end of a zone,
we need to support all POSITIVE values of steps.

Task 3050101

closes #1775

Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
@robodoo robodoo temporarily deployed to merge November 21, 2022 16:25 Inactive
@robodoo robodoo closed this Nov 21, 2022
@fw-bot fw-bot deleted the saas-15.3-fix-selection-again-no-days-off-rar branch December 5, 2022 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants