-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[#12551] Sessions page fix #12561
[#12551] Sessions page fix #12561
Conversation
Hi @DamiGbs, thanks for submitting this PR. I noted that there are a lot of formatting changes introduced. Do revert those changes in accordance to our documentation, which states under point 3 that "no unrelated changes are [to be] introduced in the branch. This includes unnecessary formatting changes". As far as I can tell, the relevant changes are only to 5 lines. Do also note that we typically align multi-line parameters to the first parameter (e.g. line 109), or add an additional tab to indicate that the parameter list is still ongoing. When we chain operations together on a variable, we also either align to the first operation or add an additional tab as a visual indicator (e.g. line 164). We will proceed to review your PR after the changes have been made. |
This reverts commit ccf4855. revert formatting changes
Hi @weiquu, should be fixed now. Looks like i had a different formatter in VsCode that changed the whole formatting of the code. |
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.
Hi @DamiGbs, thank you for your PR, do make the requested changes and re-request for a review!
src/web/app/pages-instructor/instructor-sessions-page/instructor-sessions-page.component.ts
Outdated
Show resolved
Hide resolved
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.
LGTM! Thank you for your contribution to TEAMMATES
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
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.
LGTM!
Fixes #12551
Outline of Solution
Did the same as in PR #12550. Only that i added a error Toast: