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

CB-4018 fix: hides chart settings if charts tab is not selected #2433

Closed
wants to merge 13 commits into from
Closed

CB-4018 fix: hides chart settings if charts tab is not selected #2433

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
64c4be1
CB-4018 fix: hides chart settings if charts tab is not selected
Feb 29, 2024
6a01192
CB-4018 adds to DynamicDataContext observable contexts
Mar 1, 2024
82be78b
Revert "CB-4018 adds to DynamicDataContext observable contexts"
Mar 1, 2024
0718e09
Revert "CB-4018 fix: hides chart settings if charts tab is not selected"
Mar 1, 2024
e2c3705
CB-4018 fix: hides chart settings if charts tab is not selected [2]
Mar 1, 2024
5501d9b
СB-4018 reportObserved for TempDataContext hasOwn fn
Mar 1, 2024
ba050d8
СB-4018 reportObserved for TempDataContext getOwn fn
Mar 1, 2024
e1281de
СB-4018 fix: TempDataContext mobx subscriptions works correctly
Mar 1, 2024
61bcaa0
CB-4018 atom is privated
Mar 1, 2024
8af11e2
Merge branch 'devel' into CB-4018-the-settings-button-remains-after-s…
Mar 4, 2024
c1f397d
Merge branch 'devel' into CB-4018-the-settings-button-remains-after-s…
dariamarutkina Mar 11, 2024
0d59e3a
Merge branch 'devel' into CB-4018-the-settings-button-remains-after-s…
Mar 14, 2024
33d625e
Merge branch 'devel' into CB-4018-the-settings-button-remains-after-s…
Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions webapp/packages/core-data-context/src/DataContext/DynamicDataContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export class DynamicDataContext implements IDataContext {
this.contexts = new Map();

makeObservable(this, {
contexts: observable.shallow,
fallback: observable.ref,
set: action,
clear: action,
Expand Down
1 change: 1 addition & 0 deletions webapp/packages/core-data-context/src/DataContext/TempDataContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export class TempDataContext implements IDataContext {
this.fallback = fallback;

makeObservable<this>(this, {
map: observable.shallow,
Copy link
Member

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:

  1. context added to unobservable map
  2. has call will be equal to this.map.has(context)
  3. component rendered
  4. data flushed to original context
  5. now map is empty and value is in original context
  6. 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

Copy link
Member

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;
  }

target: observable.ref,
set: action,
delete: action,
Expand Down
Loading