-
Notifications
You must be signed in to change notification settings - Fork 398
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
CB-4018 fix: hides chart settings if charts tab is not selected #2433
CB-4018 fix: hides chart settings if charts tab is not selected #2433
Conversation
@@ -28,6 +28,7 @@ export class TempDataContext implements IDataContext { | |||
this.fallback = fallback; | |||
|
|||
makeObservable<this>(this, { | |||
map: observable.shallow, |
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 map is special map that should not trigger mobx reactions, seems like problem here that we can skip subscription to the origin context (target)
example:
hasOwn(context: DataContextGetter<any>): boolean {
return this.map.has(context) || this.target.hasOwn(context);
}
has(context: DataContextGetter<any>, nested = true): boolean {
if (this.hasOwn(context)) {
return true;
}
if (nested && this.fallback?.has(context)) {
return true;
}
return false;
}
imagine this code in the render function:
...
context.set(someContext, value);
context.has(someContext);
what will happen:
- context added to unobservable
map
has
call will be equal tothis.map.has(context)
- component rendered
- data flushed to original context
- now
map
is empty and value is in original context - component hasn't subscribed to the original context and will skip all followed changes in the
context
How to fix:
lets look at hasOwn
method:
hasOwn(context: DataContextGetter<any>): boolean {
return this.map.has(context) || this.target.hasOwn(context);
}
we need to rewrite it:
hasOwn(context: DataContextGetter<any>): boolean {
const isTargetHasOwn = this.target.hasOwn(context);
return this.map.has(context) || isTargetHasOwn;
}
now we will always subscribe to the target context
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.
maybe we also need a signal
when we flush data
https://mobx.js.org/custom-observables.html
so we will call this.atom.reportObserved()
each time we access TempDataContext
and this.atom.reportChanged()
each time we flush
like this:
hasOwn(context: DataContextGetter<any>): boolean {
this.atom.reportObserved();
const isTargetHasOwn = this.target.hasOwn(context);
return this.map.has(context) || isTargetHasOwn;
}
webapp/packages/core-data-context/src/DataContext/TempDataContext.ts
Outdated
Show resolved
Hide resolved
…witching-from-charts-to-the-table
…witching-from-charts-to-the-table
…witching-from-charts-to-the-table
…witching-from-charts-to-the-table
No description provided.