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

Feature #200 - Display loader for ajax process #204

Merged
merged 14 commits into from
Mar 27, 2023

Conversation

BhargavBhandari90
Copy link
Contributor

@BhargavBhandari90 BhargavBhandari90 commented Mar 5, 2023

Description of the Change

Displaying AJAX loader for the local avatar upload delete process.

Steps:

  • Login and go to user Edit Profile Screen in the backedn
  • Upload avatar
  • You will see a loader when upload ( If you are an Adminidtrator )
  • You will see a loader when delete the local avatar

Closes #200

How to test the Change

https://somup.com/c0ehjwy3Lw

Changelog Entry

Added - Ajax loading animation during process of uploading and deleting local avatars.

Credits

Props @lllopo

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@faisal-alvi
Copy link
Member

Hi @BhargavBhandari90, I wanted to express my appreciation for your PR. After watching your demo video, I have a couple of points to share:

  1. It would be beneficial to show a loader inside the default avatar instead of displaying it on the right side.
  2. Another implementation (Local avatar delete-remove functionality added #180) is about to be merged that adds "Delete" and "Remove" buttons. "Delete" permanently deletes an image from the site, while "Remove" simply disassociates it from the user while keeping it available in the media. Will your loader also work with these buttons, or will additional code need to be added to handle them?

@BhargavBhandari90
Copy link
Contributor Author

@faisal-alvi

It would be beneficial to show a loader inside the default avatar instead of displaying it on the right side.

  • OK. I will do that.

Another implementation (

  • Not sure. I will check that.

@BhargavBhandari90 BhargavBhandari90 changed the title Feature #200 - Display loader for ajax process WIP: Feature #200 - Display loader for ajax process Mar 12, 2023
@BhargavBhandari90
Copy link
Contributor Author

This is how it looks now.

https://somup.com/c0el1lyySH

NOTE: I am still checking this with this PR: #180

@BhargavBhandari90
Copy link
Contributor Author

@faisal-alvi

Question:

  • Should I push my changes with the branch fix/41 merged in my work
    OR
  • Should I wait for that to be merged in the repo?

@faisal-alvi
Copy link
Member

@BhargavBhandari90 Awesome! thank you for your excellent work! We're almost ready to merge PR #180, and we're just waiting for some feedback from the team on my question. Once we receive that feedback, we'll merge fix/41 into develop, and then we can merge develop here.

In the meantime, I was hoping to make a small change. Currently, when the loader is displayed, the height of the div containing the avatar/loader reduces, causing the content on the page to shift vertically. I was wondering if we could apply a minimum height to the loader/avatar's parent div to avoid that shift.

sla-204-1

Thank you again for your hard work.

@lllopo
Copy link

lllopo commented Mar 14, 2023

@faisal-alvi Just a small note - I see from the demo that the whole thing "jumps" quite a lot. Maybe it needs a fixed height on the avatar container or something so the interface remains stable when it switches between loader and image.

@faisal-alvi
Copy link
Member

Thank you, @lllopo, for bringing that to my attention. I had actually requested the same change in my comment above.

@BhargavBhandari90
Copy link
Contributor Author

@faisal-alvi @lllopo Thank you guys for your input. Yah I will include that jumping thing fix in my PR.

@faisal-alvi
Copy link
Member

@BhargavBhandari90 PR #180 is closed after the discussion. It was adding "Delete" and "Remove" buttons. However, since it's closed now, we don't need to wait for it to be merged. I appreciate your patience and hard work on this PR. Please proceed with addressing the "vertical shift" issue, and once that's done, we can merge this PR. Thank you!

@BhargavBhandari90
Copy link
Contributor Author

@BhargavBhandari90 PR #180 is closed after the discussion. It was adding "Delete" and "Remove" buttons. However, since it's closed now, we don't need to wait for it to be merged. I appreciate your patience and hard work on this PR. Please proceed with addressing the "vertical shift" issue, and once that's done, we can merge this PR. Thank you!

All right. I will try to get it done by this weekend.

@BhargavBhandari90
Copy link
Contributor Author

@faisal-alvi

Let me know if I did it correct OR not.

Thanks

@BhargavBhandari90 BhargavBhandari90 changed the title WIP: Feature #200 - Display loader for ajax process Feature #200 - Display loader for ajax process Mar 26, 2023
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@BhargavBhandari90 thank you for your contribution. The video is helpful, thanks for it. I also tested it, and the loader jumping issue seems to be fixed. The new structure works fine as well.

sla-204-02

@faisal-alvi faisal-alvi merged commit e82255f into 10up:develop Mar 27, 2023
@BhargavBhandari90
Copy link
Contributor Author

@faisal-alvi Thank you

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.

Add ajax loader when adding/deleting avatar from the user profile
5 participants