search

OpenTree-Education / rhizone-lms

A learning management system focused on self-reflection.
https://rhi.zone
BSD 3-Clause Clear License
14 stars 7 forks source link

#447 Replace loading text on ProgramsPage with MUI CircularProgress #456

Closed wendyyng closed 1 year ago

wendyyng commented 1 year ago

Proposed changes

This PR resolves #447 . It replaces the "Loading..." text on ProgramsPage with MUI's CircularProgress (spinner). I referred to the SessionContext page when I made the changes to the ProgramsPage. I noticed that the spinner was not as visible as I expected so I used <CircularProgress disableShrink /> instead of <CircularProgress />.

We should also expand the scope of this PR by replacing "Loading..." text on other pages as well:

The following components currently do not include "isLoading" but may be added if necessary:

Checklist

image

wendyyng commented 1 year ago

I think I need some feedback from the team before I replace the "Loading..." text on other pages. For now I just simply copied the code from SessionContext and pasted in the ProgramsPage and it serves the purpose of indicating that the page is loading. But there are a few things we can change to make the UI better (if we want):

daveytech commented 1 year ago

Great work so far and thank you for presenting some options for us to move forward. It might be nice to see some examples in screenshots before we make a decision. Are you able to provide us with a few examples of your favourite spinners and locations?

wendyyng commented 1 year ago

Great work so far and thank you for presenting some options for us to move forward. It might be nice to see some examples in screenshots before we make a decision. Are you able to provide us with a few examples of your favourite spinners and locations?

seidior commented 1 year ago

It looks good, and it uses the same overall spinner that we're already using in SessionContext.tsx, so I think while there may be other attractive options, for now keeping consistency with that same spinner element from Material UI is good. We can always revisit this later in a maintenance ticket if we want to swap out the spinner for another one.

The only feedback that I have is that while you've adapted the spinner to better suit the ProgramsPage's different environment, there's one more tweak that I'd suggest. In SessionContext, I don't believe we've rendered the Navbar element yet, so it makes sense to have the spinner be in the direct middle and center of the page; however, in ProgramsPage, I think the spinner effectively ends up too far down on the page.

I tweaked and played around with it, and I think it's better in this case to switch from viewport units to text size units, specifically changing the Stack in ProgramsPage.tsx (on line 34) from a height of "100vh" to a height of "40em". I tested this at different text sizes and in different window sizes, and it puts the spinner consistently in the middle and center of the calendar component, which would be my expectation of where the spinner would appear on this page and makes it so the user's eyes don't focus on what ends up being a blank area of the page once it ends up loading.

Try it out on your machine and let me know what you think and if you would adjust the value or not. Once we settle on that, I think you're clear to amend Questionnaire.tsx because it erroneously used the loading text instead of the spinner and it's a super simple and straightforward replacement because its structure is nearly identical to our ProgramsPage.tsx, and because it's a presentation-only change that won't impact the functionality of the component (in which case if it did, I would recommend a maintenance ticket).

As far as ReflectionsPage.tsx, Meeting.tsx, CompetenciesPage.tsx, and DocPage.tsx are concerned, the replacement or addition aren't as clearly straightforward, so I'll leave it up to @daveytech if he wants to clear you for modifying those pages or if he'd recommend you open a maintenance ticket for those pages and have you move on to helping us out on #457.

wendyyng commented 1 year ago

It looks good, and it uses the same overall spinner that we're already using in SessionContext.tsx, so I think while there may be other attractive options, for now keeping consistency with that same spinner element from Material UI is good. We can always revisit this later in a maintenance ticket if we want to swap out the spinner for another one.

The only feedback that I have is that while you've adapted the spinner to better suit the ProgramsPage's different environment, there's one more tweak that I'd suggest. In SessionContext, I don't believe we've rendered the Navbar element yet, so it makes sense to have the spinner be in the direct middle and center of the page; however, in ProgramsPage, I think the spinner effectively ends up too far down on the page.

I tweaked and played around with it, and I think it's better in this case to switch from viewport units to text size units, specifically changing the Stack in ProgramsPage.tsx (on line 34) from a height of "100vh" to a height of "40em". I tested this at different text sizes and in different window sizes, and it puts the spinner consistently in the middle and center of the calendar component, which would be my expectation of where the spinner would appear on this page and makes it so the user's eyes don't focus on what ends up being a blank area of the page once it ends up loading.

Try it out on your machine and let me know what you think and if you would adjust the value or not. Once we settle on that, I think you're clear to amend Questionnaire.tsx because it erroneously used the loading text instead of the spinner and it's a super simple and straightforward replacement because its structure is nearly identical to our ProgramsPage.tsx, and because it's a presentation-only change that won't impact the functionality of the component (in which case if it did, I would recommend a maintenance ticket).

As far as ReflectionsPage.tsx, Meeting.tsx, CompetenciesPage.tsx, and DocPage.tsx are concerned, the replacement or addition aren't as clearly straightforward, so I'll leave it up to @daveytech if he wants to clear you for modifying those pages or if he'd recommend you open a maintenance ticket for those pages and have you move on to helping us out on #457.

Thank you so much for the suggestions! I changed the height to "40em" and I think it looks good on the page. I also replaced the loading text on Questionnaire - the "How are you feeling about your current endeavors" questionnaire on the home page