labrocadabro / communitytaught

MIT License
79 stars 31 forks source link

Add progress meter #19

Open 7MinutesDead-Git opened 1 year ago

7MinutesDead-Git commented 1 year ago

I've added a very basic progress meter to the All Classes page. At the moment, it's purely client-side with basic styling (to match Leon's stream background animation). image

It works by counting the amount of lessons marked as completed on the page, as well as updating when a checkbox is ticked or unticked.

Since this is only client-side logic for now, we can't add this progress bar to the Dashboard view yet. However, this should be easily doable server-side if a query was retrieved for the amount of Lessons marked completed, served as the initial progress bar width value, and then the client-side js could keep the bar's visuals in sync between refreshes (if the user clicks some more boxes after).

Let me know what you think, and if any changes need to be made!

Some TODOs are to make for a smooth animation for the bar to fill up on page load, and to smoothly adjust as checkboxes are clicked.

(Also, if you later end up rebuilding this app in React, I can rebuild the progress bar in React as well where all of this is quite a bit simpler)

7MinutesDead-Git commented 1 year ago

Absolutely!

7MinutesDead-Git commented 1 year ago

I've converted the styling to the builtin tailwind classes to align with the existing style of the site, as well as adding some transition to when the progress meter updates.

image

7MinutesDead-Git commented 1 year ago

it needs to be hidden when the user isn't logged in.

Got it!

Can you add a bar for the homework page as well? I think it should be pretty trivial since you've got most of the code written already.

This turned out to be non-trivial as the client-side logic wasn't applicable to the dashboard page since the dashboard doesn't have all the lessons rendered. I went ahead and converted the logic to get the initial lesson and completed lesson counts server-side and pass those along to the templates. It's now working! Take a look.

I'd like the progress text (x/54) in the center of the bar and have the text color based on the background color (white text on the pink background, dark blue text on the light blue background). An example (and solution) here: > https://stackoverflow.com/questions/21909494/how-to-create-a-progressbar-with-inverted-text-color-for-progressed-part

This one is a bit tricky. We could turn this into an issue / todo and someone could knock it out later! It should be possible, but it might take further adjusting of the progressBar mixin structure and how the text values are handled client-side on the allLessons page by the js, as currently they're innerText of the meter divs themselves.

labrocadabro commented 1 year ago

This turned out to be non-trivial as the client-side logic wasn't applicable to the dashboard page since the dashboard doesn't have all the lessons rendered. I went ahead and converted the logic to get the initial lesson and completed lesson counts server-side and pass those along to the templates. It's now working! Take a look.

I actually wasn't talking about the dashboard page, I was talking about the homework page (https://communitytaught.org/hw/all). As a side note, the dashboard progress bar is not currently working - it's just a missing include of the JS file, though.

While there is a case to be made that the progress bar should increment as soon as classes are watched, it does have a couple of issues: 1) checking in is a required part of completing the program and getting verification. I am not sure how verification will work (if at all) now that we're starting to look to cohort 3, but I'd like it to remain as is for now, at least. 2) it introduces an inconsistency between the progress bar and when the classes are marked "done" (that requires both watching and checking in, for the above mentioned reasons).

Unfortunately, some classes require both watching and checking in, and some require just watching. Classes that don't require checking in are marked "checkedin: false" by default, meaning you would have to add more complex logic to keep the front and back end information consistent. Additionally, a similar strategy will not work for homework tracking, which is much more complicated.

There are three solutions I can see: 1) Roll back to the fully client-side solution and remove the progress bar from the dashboard page. I do believe that adding the homework tracking would be fairly trivial in that case, as you can simply count the number of total items and the number that are done on the client side. You could add it yourself or leave it for a different PR. This is my preferred choice. 2) Change the "done" logic on the client side to care about watched status only. Keep your PR as it is currently, add the progress bar to the class and dashboard pages only, and leave the homework page progress tracking for a different PR. I don't prefer this solution as I explained, but If that's the way you'd like to go I'm ok with it. 3) Work out a proper backend solution that would involve saving the done status to the database. This is the long-term solution but it requires fairly significant changes and a script to update all existing progress for all users. That is outside the scope of this PR and I'm mentioning it here just for completeness.

This one is a bit tricky. We could turn this into an issue / todo and someone could knock it out later! It should be possible, but it might take further adjusting of the progressBar mixin structure and how the text values are handled client-side on the allLessons page by the js, as currently they're innerText of the meter divs themselves.

Understood, I thought it might need to go to a separate issue.

Please decide which of the two options you'd like to go with for the progress bar logic and submit a final PR.

Everything really does look great, thank you so much for sticking with this. :)