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

Fix remaining frontend issues for pagination #23

Conversation

teikjun
Copy link

@teikjun teikjun commented Feb 27, 2021

Part of TEAMMATES#10959, follow-up to #22.

Outline of Solution
Make requested changes to TEAMMATES#10960

  • Rename 'download' to 'loading' for general services and remove empty file
  • Close loading modal upon failure.
  • Fix overlay bug on chrome

@teikjun teikjun changed the title 10959 paginate frontend call to webapi for large course Make requested changes Feb 27, 2021
@teikjun teikjun changed the title Make requested changes Fix remaining frontend issues for pagination Feb 27, 2021
@teikjun
Copy link
Author

teikjun commented Feb 28, 2021

Ready for review!
@moziliar @daongochieu2810 Please help to take a look whenever you're free.

Copy link
Collaborator

@daongochieu2810 daongochieu2810 left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -30,7 +30,7 @@ <h5 class="modal-title">
<button type="button" class="btn btn-light modal-btn-cancel" *ngIf="!isInformationOnly" (click)="activeModal.dismiss()">
{{ cancelMessage }}
</button>
<button type="button" class="btn modal-btn-ok"
<button type="button" class="btn modal-btn-ok z-index-1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of class z-index-1, how about style="z-index: 1" just to be cleaner

Copy link
Author

@teikjun teikjun Feb 28, 2021

Choose a reason for hiding this comment

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

Thanks for the review. I was also considering to inline the style as suggested!

However, I saw on line 13 (in the same file):

<span class="margin-left-5px" [innerHTML]="header"></span>

So, I thought it would be better to be consistent with the convention used in the original code.

@teikjun
Copy link
Author

teikjun commented Mar 1, 2021

@moziliar Feel free to merge this into the main PR if everything looks fine! :)

Copy link
Owner

@moziliar moziliar left a comment

Choose a reason for hiding this comment

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

LGTM

@moziliar moziliar merged commit 652eeed into moziliar:10959-paginate-frontend-call-to-webapi-for-large-course Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants