-
Notifications
You must be signed in to change notification settings - Fork 27
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
ScrollService #344
base: master
Are you sure you want to change the base?
ScrollService #344
Conversation
src/core/scroll/scroll.service.js
Outdated
import { AppError } from '../infrastructure/error'; | ||
import { jobLine } from '../services/job.line'; | ||
|
||
const OFFSET = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this properties under the scroll.model
@@ -37,7 +38,8 @@ export class BodyCoreComponent extends NgComponent implements OnInit { | |||
|
|||
const { model } = this.root; | |||
const table = this.$table; | |||
const ctrl = new BodyCtrl(model, view, this.root.table, this.root.bag); | |||
const scrollService = new ScrollService(model, this.root.table); | |||
const ctrl = new BodyCtrl(model, view, scrollService, this.root.table, this.root.bag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instantiate scrollService inside the BodyCtrl
src/core/scroll/scroll.service.js
Outdated
const VELOCITY = 5; | ||
|
||
export class ScrollService extends Disposable { | ||
constructor(model, table) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disposable service is not good practice, move all disposables outside of the service and body ctrl
src/core/scroll/scroll.service.js
Outdated
} | ||
|
||
doScroll(direction) { | ||
const scrollState = this.model.scroll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const {scroll} = this.model;
src/core/body/body.ctrl.d.ts
Outdated
|
||
export declare class BodyCtrl { | ||
constructor(model: Model, view: any, table: Table, bag: any); | ||
|
||
scrollService: ScrollService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hide this property
src/core/body/body.ctrl.js
Outdated
@@ -2,6 +2,7 @@ import { PathService } from '../path/path.service'; | |||
import { Fastdom } from '../services/fastdom'; | |||
import { GRID_PREFIX } from '../definition'; | |||
import { jobLine } from '../services/job.line'; | |||
import { ScrollService } from 'ng2-qgrid/core/scroll/scroll.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont use ng2-qgrid in core use relative path
src/core/scroll/scroll.service.js
Outdated
} | ||
|
||
canScroll(e) { | ||
const table = this.rect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just call it rect: const {rect, offset} = this;
export declare class ScrollService { | ||
constructor(model: Model, table: Table); | ||
|
||
stop(): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add scroll method
src/core/scroll/scroll.service.js
Outdated
e.clientX < (table.right - offset)) | ||
} | ||
|
||
scroll(e, startCell) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to start
src/core/scroll/scroll.service.js
Outdated
|
||
} | ||
|
||
doScroll(direction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to croll
src/core/scroll/scroll.service.js
Outdated
const table = this.rect; | ||
const offset = this.offset; | ||
|
||
return !(e.clientY < (table.bottom - offset) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split this to var defs:
const is... =
src/core/scroll/scroll.service.js
Outdated
const { velocity } = scrollState; | ||
const scrolledToEnd = () => this.isScrolledToEnd(direction); | ||
|
||
return () => setInterval(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to use job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to tricky, we need to make own interval like a jobLine, or emulate it with job
src/core/scroll/scroll.service.js
Outdated
this.view = view; | ||
this.interval = null; | ||
this.rect = null; | ||
this.body = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use this.markup.body directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markup is not available during the ScrollService initialization
@@ -56,7 +59,14 @@ export class BodyCoreComponent extends NgComponent implements OnInit { | |||
}); | |||
|
|||
listener.on('mousedown', ctrl.onMouseDown.bind(ctrl)); | |||
listener.on('mouseup', ctrl.onMouseUp.bind(ctrl)); | |||
|
|||
windowListener.on('resize', () => ctrl.resize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigate if it could be run outsize ng zone
get offset() { | ||
return this.model.scroll().offset; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write unit tests
src/core/scroll/scroll.service.js
Outdated
model.scrollChanged.watch(e => { | ||
if (this.interval) { | ||
const path = this.getPath(this.mouseEvent); | ||
const td = pathFinder.cell(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could it be that we do not find td?
src/core/scroll/scroll.service.js
Outdated
const offset = this.offset; | ||
|
||
// check if mouse is not in areas which fire scrolling | ||
const mouseNotOnTopSide = e.clientY > (rect.top + offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could me make positive booleans? like cursorIsOnTop?
} | ||
|
||
start(e, startCell) { | ||
this.mouseEvent = e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we calculate td here? not in scrollChanged event
src/core/scroll/scroll.service.js
Outdated
this.mouseEvent = e; | ||
this.startCell = startCell; | ||
|
||
if (this.interval) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how it could happen on start?
src/core/scroll/scroll.service.js
Outdated
if (direction && !this.interval) { | ||
this.job(() => { | ||
const interval = this.scroll(direction); | ||
this.interval = interval(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we emulate interval with jobs?
src/core/scroll/scroll.service.js
Outdated
const { velocity } = scrollState; | ||
const scrolledToEnd = () => this.isScrolledToEnd(direction); | ||
|
||
return () => setInterval(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to tricky, we need to make own interval like a jobLine, or emulate it with job
No description provided.