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

improve data events #84

Merged
merged 13 commits into from
Jul 16, 2020
Merged

improve data events #84

merged 13 commits into from
Jul 16, 2020

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented Jul 15, 2020 •
edited
Loading

fix https://app.clubhouse.io/cartoteam/story/92672/map-refine-map-related-data-events

The idea is to create a state object which is responsible of:

  • getting event inputs: from deck instance (zoom, pan, rotate, tiles map loaded and onAfterRender)
  • emitting 2 new events: 'layer:data:ready' and layer:data:changed' when is required

The objectives are:

  • solving the problems
  • manage the events in one place

By now it is also emitting the previous one viewportLoad which is used by dataviews, but the idea is to change to the new ones.

@vercel
Copy link

vercel bot commented Jul 15, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carto-frontend/web-sdk/msi1n2rwv
✅ Preview: https://web-sdk-git-bug-ch89765improve-data-events.carto-frontend.vercel.app

@simon-contreras-deel simon-contreras-deel force-pushed the bug/ch89765/improve-data-events branch from 34b9426 to 951dc3e Compare July 15, 2020 16:44
VictorVelarde
VictorVelarde reviewed Jul 15, 2020
View reviewed changes
src/lib/viz/layer/Layer.ts Outdated
@@ -21,6 +22,16 @@ import { FunctionFilterApplicator } from '../filters/FunctionFilterApplicator';
import { ColumnFilters } from '../filters/types';
import { basicStyle } from '../style/helpers/basic-style';

const DATA_READY_EVENT = 'layer:data:ready';
Copy link
Contributor

Choose a reason for hiding this comment

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

deckgl uses 'camelCase', so how about 'layerDataReady'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have not a real preference

VictorVelarde
VictorVelarde reviewed Jul 15, 2020
View reviewed changes
src/lib/viz/layer/Layer.ts Outdated

function buildInitialDataState() {
return {
isFirstTime: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we join 'isPanning' / 'isZooming'... we might consider using an enum instead of a set of flags, so only 1 state is possible at the same time...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think it is possible to do so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2 steps if you want to follow the code

@vercel vercel bot temporarily deployed to Preview July 16, 2020 07:53 Inactive
@simon-contreras-deel simon-contreras-deel force-pushed the bug/ch89765/improve-data-events branch from 7523706 to 8dc2177 Compare July 16, 2020 07:54
@simon-contreras-deel simon-contreras-deel marked this pull request as ready for review July 16, 2020 08:01
VictorVelarde
VictorVelarde reviewed Jul 16, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
VictorVelarde
VictorVelarde reviewed Jul 16, 2020
View reviewed changes
src/lib/viz/layer/Layer.ts
@@ -94,6 +107,7 @@ export class Layer extends WithEvents implements StyledLayer {

if (this._deckLayer) {
await this.replaceDeckGLLayer();
this.dataState = DATA_STATES.STARTING;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is applied to setSource. Would it be also necessary with setStyle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it would not because data does not change with setStyle

VictorVelarde
VictorVelarde approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM. Small suggestion of renaming 'layerDataReady' --> 'dataReady'

@simon-contreras-deel simon-contreras-deel merged commit 5bbb758 into develop Jul 16, 2020
@simon-contreras-deel simon-contreras-deel deleted the bug/ch89765/improve-data-events branch July 16, 2020 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VictorVelarde VictorVelarde VictorVelarde approved these changes

@jesusbotella jesusbotella Awaiting requested review from jesusbotella

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

Successfully merging this pull request may close these issues.
2 participants
@simon-contreras-deel @VictorVelarde