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.

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

Danyro/teacher onboarding bugs and update #101

Merged
merged 50 commits into from
Sep 20, 2020

Conversation

danyrojj
Copy link
Contributor

No description provided.

Comment on lines 9 to 11
firstName: string;
lastName: string;
email: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LolaRev @danyro0 why do we need these properties? we already have those in user's info. can be edited elsewhere.
can teacher have these set to different values from their user account?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this decision might also affect the UI

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LolaRev @danyro0 why do we need these properties? we already have those in user's info. can be edited elsewhere.
can teacher have these set to different values from their user account?

As mentioned, it was before we move it to the sessions. I think that we can remove the view but let's keep the shortcut for creating a session @yinon @danyro0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yinonov it's a final data model for the onboarding flow, i think we need this interface only for giving a type to post request when we implement submitting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, it's a nice feature, we already has a setup for it. @danyro0 we discussed it.

can we remove this step then?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I'll remove it

@@ -22,9 +23,9 @@ export const ACTIVITIES = 'activities';
export class ActivitiesBoxComponent implements OnInit, OnDestroy, AfterViewInit {
@Input() pillar;
public form: FormGroup;
formValueChanges$;
formValueChanges$: Subscription;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we handle subscriptions with untilDestroy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! way cooler

@ViewChild('pillarList') pillarList: PillarListComponent;
pillarEnum = Pillar;
formValueChanges$;
formValueChanges$: Subscription;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we handle subscriptions with untilDestroy

@@ -19,8 +22,8 @@ export class SessionTypesStepComponent implements OnDestroy {
form: FormGroup;
typesEnum = SessionTypes;
sessionTypesData = sessionTypesData;
formValueChanges$;
controlKey = USER_ONBOARDING + '-' + SESSION_TYPES;
formValueChanges$: Subscription;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to untilDestroy

Comment on lines 28 to 29
get typeKeys() {
return Object.keys(this.typesEnum);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we try to follow the literalMapping approach as we did for other types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yinonov do you think we need to implement it everywhere instead of this enum,keys pattern? i thinks it adds complexity for some cases like the activity-box

@@ -1,3 +0,0 @@
.grid-item {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's deleted :)

}

getCache() {
this.storage.getItem(this.controlKey).subscribe((cacheValue) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this ever unsubscribes?

this.initFormWithCachedData(teacher);
}
subscribeToValueChanges() {
this.formValueChanges$ = this.form.valueChanges.subscribe((value) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use untilDestroy

@danyrojj danyrojj mentioned this pull request Sep 16, 2020
Copy link
Collaborator

@yinonov yinonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danyro0 can u please merge live and resolve conflicts to proceed with the PR merge?

@@ -83,17 +84,13 @@ export class ActivitiesBoxComponent implements OnInit, OnDestroy, AfterViewInit
}

ngAfterViewInit(): void {
this.formValueChanges$ = this.form.valueChanges.subscribe((changedVal) => {
this.form.valueChanges.subscribe((changedVal) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to add this operator where dependent on untilDestory

Suggested change
this.form.valueChanges.subscribe((changedVal) => {
this.form.valueChanges.pipe(
untilDestroyed(this)
).subscribe((changedVal) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh, yep, gotta

@yinonov yinonov merged commit 56845a5 into live Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants