Mobile 4842 #4554
Mobile 4842 #4554
Conversation
crazyserver
force-pushed
the
MOBILE-4842
branch
5 times, most recently
from
be9f856 to
5443d3f
Compare
crazyserver
force-pushed
the
MOBILE-4842
branch
from
5443d3f to
071be74
Compare
| effect((() => { | ||
| this.offline.set(!this.isOnline()); | ||
| })); |
There was a problem hiding this comment.
Instead of an effect, you can use a linkedSignal. linkedSignal is like a computed signal, but then you can override the value.
| protected async networkChanged(): Promise<void> { | ||
| const wasOnline = this.isOnline; | ||
| this.isOnline = CoreNetwork.isOnline(); | ||
|
|
||
| if (this.isOnline && this.offlineErrorAlert) { | ||
| // Back online, dismiss the offline error alert. | ||
| this.offlineErrorAlert.dismiss(); | ||
| this.offlineErrorAlert = null; | ||
| } | ||
|
|
||
| if (this.playing && !this.fileUrl && !this.isOnline && wasOnline && this.trackComponent) { | ||
| // User lost connection while playing an online package with tracking. Show an error. | ||
| this.offlineErrorAlert = await CoreAlerts.showError( | ||
| new CoreError(Translate.instant('core.course.changesofflinemaybelost'), { | ||
| title: Translate.instant('core.youreoffline'), | ||
| }), | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (this.isOnline && this.triedToPlay) { | ||
| // User couldn't play the package because he was offline, but he reconnected. Try again. | ||
| this.triedToPlay = false; | ||
| this.play(); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Why did you delete all this code? It was important to improve the UX for users that lose connection or re-connect.
| /** | ||
| * Returns a signal to watch if the device uses a cellular connection. | ||
| * | ||
| * @returns Signal. | ||
| */ | ||
| isCellularSignal(): Signal<boolean> { | ||
| return computed<boolean>(() => this.connectionTypeSignal()() === CoreNetworkConnectionType.CELL); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a signal to watch if the device uses a wifi connection. | ||
| * | ||
| * @returns Signal. | ||
| */ | ||
| isWifiSignal(): Signal<boolean> { | ||
| return computed<boolean>(() => this.connectionTypeSignal()() === CoreNetworkConnectionType.WIFI); | ||
| } |
There was a problem hiding this comment.
Related to this and some existing code in this service, some weeks ago we talked about signals in CoreKeyboard and we decided to use a getter to retrieve a signal value, because we introduced a bug by mistake because we only used one parenthesis instead of 2. E.g. we used keyboardShownSignal() instead of keyboardShownSignal()(). With the getter, to obtain the value you only need to do: keyboardShownSignal().
I guess this is something we should do in this service (and other services where we are using signals, if any) for concistency. So if onlineSignal is a getter, then in the components where you want to obtain the signal you need to do CoreNetwork.onlineSignal (instead of CoreNetwork.onlineSignal()), and if you want to obtain the value then you can read it with CoreNetwork.onlineSignal() instead of CoreNetwork.onlineSignal()(). The same applies to connectionTypeSignal.
| /** | ||
| * Returns a signal to watch if the device uses a cellular connection. | ||
| * | ||
| * @returns Signal. | ||
| */ | ||
| isCellularSignal(): Signal<boolean> { | ||
| return computed<boolean>(() => this.connectionTypeSignal()() === CoreNetworkConnectionType.CELL); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a signal to watch if the device uses a wifi connection. | ||
| * | ||
| * @returns Signal. | ||
| */ | ||
| isWifiSignal(): Signal<boolean> { | ||
| return computed<boolean>(() => this.connectionTypeSignal()() === CoreNetworkConnectionType.WIFI); | ||
| } |
There was a problem hiding this comment.
Also in this code, every time isCellularSignal or isWifiSignal are called you're creating a new computed signal. Wouldn't it be better to have these computed signals as properties in the service and then just return them? With a getter as I mention in the previous comment.
This change means we'll always be calculating these values even if they aren't being used, but they're very simple so IMO it's not a big deal. And that allows you to use these functions anywhere, not just in a constructor or injection context like it happens with your implementation.
|
|
||
| protected breakpointsSubject: BehaviorSubject<Record<Breakpoint, boolean>>; | ||
| private _layoutObservable: Observable<CoreScreenLayout>; | ||
| readonly orientationSignal = signal<CoreScreenOrientation>(this.orientation); |
There was a problem hiding this comment.
Do we want this signal to be public? We usually make signals in services protected or private and then we expose as readonly using a getter/function. Otherwise anyone can change the value of this signal.
| <ion-col> | ||
| @if (criteria.timecompleted) { |
There was a problem hiding this comment.
Before this change, if there was no timecompleted, the ion-col wasn't displayed. Now it will be displayed but empty. Is that ok? Just to double check it's a deliberate decision.
| try { | ||
| this.courseId = CoreNavigator.getRequiredRouteNumberParam('courseId'); | ||
| this.userId.set(CoreNavigator.getRouteNumberParam('userId') || CoreSites.getCurrentSiteUserId()); | ||
| } catch (error) { | ||
| CoreAlerts.showError(error); | ||
| CoreNavigator.back(); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
AFAIK we always read parameters in ngOnInit, since that's when the Angular component is considered "ready to be initialized". I'm not sure if we have any case where we read route params in constructor, or if this can cause problems (e.g. parameters not ready in the constructor).
I would keep the code in ngOnInit just in case, and also to keep it consistent. A third reason is that we usually try not to call services in constructor, because that can cause "true" circular dependencies at runtime, breaking the app.
I guess you moved this code in the constructor to be able to define courseId as a constant. I would just define the courseId as a signal and that's it, even if it doesn't change later.
No description provided.