Skip to content

[FIX] Spreadsheet: Fix scroll issues in mobile#6082

Closed
rrahir wants to merge 1 commit into18.0from
18.0-scroll-mobile-rar
Closed

[FIX] Spreadsheet: Fix scroll issues in mobile#6082
rrahir wants to merge 1 commit into18.0from
18.0-scroll-mobile-rar

Conversation

@rrahir
Copy link
Copy Markdown
Collaborator

@rrahir rrahir commented Apr 11, 2025

Description:

Currently, the scroll via touch is barely supported and only on the grid overlay DOM element, which means that you cannot scroll if you pass your finger on a figure for instance.

This task tries to help out specifically in dashboard mode as there are no interactions with the different components (no drag and drop of components, grid selection, etc...).

Task: 4720073

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

Task: 4720073

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 (_t("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
Copy Markdown
Collaborator

robodoo commented Apr 11, 2025

Pull request status dashboard

@rrahir rrahir force-pushed the 18.0-scroll-mobile-rar branch from e4549b9 to 144566d Compare April 11, 2025 09:09
Copy link
Copy Markdown
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.

👋 tchou tchou


const HorizontalScrollFactor = 1;

// TODO merge it with the stop scroll of lucas
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

todo

@@ -7,6 +7,7 @@ import { GridPopover } from "../grid_popover/grid_popover";
import { css, cssPropertiesToCss } from "../helpers/css";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Other commit seems useless, the isMobile helpers are never used

Comment on lines +6 to +8
const verticalScrollFactor = 1;

const HorizontalScrollFactor = 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These factors seem useless is the value is 1 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I kept them in case i should fine tune through the testing phase


const verticalScrollFactor = 1;

const HorizontalScrollFactor = 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const HorizontalScrollFactor = 1;
const horizontalScrollFactor = 1;

or all caps.

@rrahir rrahir force-pushed the 18.0-scroll-mobile-rar branch from 144566d to 9402c7e Compare April 15, 2025 11:15
Copy link
Copy Markdown
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.

Looking good 👍
only a small question.

Comment on lines +29 to +30
const clientX = ev instanceof MouseEvent ? ev.clientX : ev.touches[0].clientX;
const clientY = ev instanceof MouseEvent ? ev.clientY : ev.touches[0].clientY;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

in which case is this a MouseEvent or a TouchEvent ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good question, it should be a touchevent 100% of the time so that helper doesn't really make sense .. i'll loot into it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@LucasLefevre I removed the helper, it really was useless

Currently, the scroll via touch is barely supported and only on the
grid overlay DOM element, which means that you cannot scroll if you
pass your finger on a figure for instance.

This task tries to help out specifically in dashboard mode as there
are no interactions with the different components (no drag and drop
of components, grid selection, etc...); in edition mode, the mobile
is still barely usable but this will be addressed in master considering
the number of modifications required.

Task: 4720073
@rrahir rrahir force-pushed the 18.0-scroll-mobile-rar branch from 9402c7e to 5e4da6d Compare April 18, 2025 10:10
Copy link
Copy Markdown
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+

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.

4 participants