Mobile 4842 #4538
Mobile 4842 #4538
Conversation
crazyserver
force-pushed
the
MOBILE-4842
branch
9 times, most recently
from
a025d96 to
f6ce1e1
Compare
crazyserver
force-pushed
the
MOBILE-4842
branch
2 times, most recently
from
8085cb3 to
1dd09d4
Compare
crazyserver
force-pushed
the
MOBILE-4842
branch
3 times, most recently
from
22cb6f7 to
b371770
Compare
crazyserver
force-pushed
the
MOBILE-4842
branch
from
b371770 to
01e30c4
Compare
| export enum CoreBlocksRegion { | ||
| CONTENT = 'content', | ||
| MAIN = 'main', | ||
| SIDE = 'side', // Used as a prefix for side blocks. (site-pre, side-post). |
There was a problem hiding this comment.
Typo: site-pre shoud be side-pre I guess.
| visible: true, | ||
| })); | ||
|
|
||
| this.hasSideBlocks = CORE_BLOCKS_DASHBOARD_FALLBACK_BLOCKS.some((blockName) => |
There was a problem hiding this comment.
Why did you change hasMainBlocks with hasSideBlocks? Looking at the code, the fallback blocks are added as main blocks, not side blocks.
| @@ -436,14 +437,15 @@ export class CoreCoursesHelperProvider { | |||
|
|
|||
| /** | |||
| * Check whether completion is available in a certain course. | |||
| * This is a temporary function to be used until we move AddonCourseCompletion to core folder (MOBILE-4537). | |||
| * This is a temporary function to be used until we move CoreCourseCompletionHelper to core folder (MOBILE-4537). | |||
There was a problem hiding this comment.
This comment is a bit confusing, since CoreCourseCompletionHelper is now in core folder. Is MOBILE-4537 still needed after these changes?
| * Service that provides some features regarding a course completion. | ||
| */ | ||
| @Injectable({ providedIn: 'root' }) | ||
| export class CoreCourseCompletionHelperService { |
There was a problem hiding this comment.
In this case IMO we can remove the "Helper" word and just name it CoreCourseCompletionService. AFAIK the "Helper" services were created mainly for 2 reasons:
- To avoid having too much code in a single service.
- To put some "advanced" logic that could look weird in the base service. E.g. prefetching courses.
In this case we don't have any advanced logic and this service is the only completion service, so I wouldn't use the Helper word.
Lately I have some mixed feelings about our helpers, because the "Helper" word doesn't provide much info on what it does (it's not easy to understand the difference between CourseHelperService and CourseService without looking at their implementations). If we continue the trend of splitting code into different services maybe in the future we can get rid of our Helper services and have better naming.
| import { makeSingleton } from '@singletons'; | ||
|
|
||
| /** | ||
| * Service that provides some features regarding a course completion. |
There was a problem hiding this comment.
Maybe we can add a comment indicating that this service is only for course completion, not activity completion in a course. Since both things can be confused.
E.g. in our Course service we have the type CoreCourseCompletionActivityStatus, but that type doesn't belong here because it's for activity completion, not course completion.
| readonly iframeRef = viewChild<ElementRef<HTMLIFrameElement>>('iframe'); | ||
| readonly iframe = computed(() => { | ||
| const iframe = this.iframeRef()?.nativeElement; | ||
| this.initIframeElement(iframe); |
There was a problem hiding this comment.
I was wondering whether the "initFrameElement" should be part of the computation or not in terms of code organization. E.g. instead of calling it here, the computed could just do:
readonly iframe = computed(() => this.iframeRef()?.nativeElement);
And then you can have an effect listening to this.iframe() and calling initIframeElement when it changes. This way the computation function is simpler and only calculates the variable itself, and other code that depends on the variable is placed in an effect. What do you think about this?
| if (!swiper) { | ||
| return; | ||
| } | ||
| this.swiper.set(swiper); |
There was a problem hiding this comment.
Here we have the same problem I mentioned in core-tabs (reading and writing same signal), maybe it's better to leave this component unchanged for now.
| return; | ||
| } | ||
|
|
||
| this.toolbarSlides.set(swiper); |
There was a problem hiding this comment.
Reading and setting the same signal here too.
| if (this.currentLogin) { | ||
| effectRef.destroy(); | ||
| } | ||
| },{ manualCleanup:true }); |
There was a problem hiding this comment.
I'm a bit confused about this manualCleanup. According to the documentation:
Whether the effect should require manual cleanup.
If this is false (the default) the effect will automatically register itself to be cleaned up with the current DestroyRef.
If this is true and you want to use the effect outside an injection context, you still need to provide an Injector to the effect.
The second line makes me think that, if it's true, then the effect will NOT be destroyed automatically, and we don't want that. In a video tutorial I found (not official), they said the same thing, using manualCleanup=true means the effect will not be destroyed automatically.
So I decided to try it. I created an effect with manualCleanup=true in a page that listened to a signal in a service. I entered the page, I left it and the effect stoped reacting to changes in my service signal. So it was automatically destroyed even if manualCleanup was true, which contradicts what was said before.
However, I also tried using effectRef.destroy() in an effect that used manualCleanup=false and it worked fine too. So I'm not sure what this manualCleanup is for.
IMO we should either not use manualCleanup if it's not needed to destroy the effect, OR use manualCleanup=true but also call effectRef.destroy in the component's OnDestroy to be sure it has been destroyed. The second one is probably safer.
| return; | ||
| } | ||
|
|
||
| this.swiper.set(swiper); |
There was a problem hiding this comment.
This effect also reads and writes the same signal.
No description provided.