-
Notifications
You must be signed in to change notification settings - Fork 376
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
Support time window query #6872
Conversation
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.
@mwu2018 - I have partially reviewed the PR and think it is a great start and good work on the specs.
In the context of ArcGis MapServer, intervals
actually mean the time step between two dates in the date selector (or the timeline). Whereas the concept we are implementing here - the window of data rendered on the map - is called a time window. So ideally we should rename the traits to something that aligns with the original terms.
Another thing we could do is combine the units and time window setting into one. We already use momentjs in a few places so you could use that to parse a string like 10s
or 1M
.
If we do that you could name the trait as timeWindowDuration
and accept strings like 1M
1Y
. The direction trait could be named as timeWindowDirection
.
Let me know what you think.
The doc says momentjs is not recommended for new projects. |
@na9da I've revised partially based on your comments. Happy to discuss. |
private getTimeParams(): TimeParams { | ||
const currentTime = this.getCurrentTime(); | ||
const timeWindowDuration = this.timeWindowDuration; | ||
const timeWindowUnit = this.timeWindowUnit; | ||
const isForwardTimeWindow = this.isForwardTimeWindow; | ||
const timeParams = { | ||
currentTime, | ||
timeWindowDuration, | ||
timeWindowUnit, | ||
isForwardTimeWindow | ||
} as TimeParams; | ||
return timeParams; | ||
} | ||
|
||
const imageryProvider = this._createImageryProvider(dateAsUnix); | ||
@computed | ||
private get _currentImageryParts(): ImageryParts | undefined { | ||
const timeParams = this.getTimeParams(); | ||
const imageryProvider = this._createImageryProvider(timeParams); |
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.
The getTimeParams()
needs to be a @computed
. This is because we are passing it as parameter to _createImageryProvider
which itself is a computed (memoized) object. But here, getTimeParams
returns a new object each time which will result in re-computing of _createImageryProvider
. This is an expensive operation. You can avoid this by converting it to something like @computed get timeParams() {...}
.
To see this in action, open the network tab for both the before and after links and try changing the opacity. For the after link, see that changing the opacity results in network fetches, whereas the network tab is silent when doing this for the before link.
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.
Great work @mwu2018 - looks good to go 🚀
What this PR does
Fixes #6873
Support time window query for esri-mapServer type.
timeWindowDuration
andtimeWindowUnit
traits to typeesri-mapServer
to support time query with window.timeWindowDuration
indicate forward time window while negative value backward window.timeWindowUnit
can be any units recognised bymoment
module. If an invalid value is given, will query time without window.Test me
before
after
Checklist
doc/
.