Add progress meter #19

7MinutesDead-Git · 2023-02-16T23:59:29Z

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).

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)

…aught into progress-meter

labrocadabro

This looks pretty good (and I see you caught a couple of my mistakes!). Unfortunately the "done" logic is currently all on the client side (I am trying to rework the back end to change that) so for the moment it will have to stay on the classes page only.

I would appreciate it if you could switch the CSS to Tailwind (and if you're not comfortable with that, you need to move the existing CSS to tailwind.css as index.css is auto-generated by Tailwind and your changes would be removed on the next compile) and make the colors a bit more muted + consistent with the current site design (the progress bar shouldn't distract from the class cards).

7MinutesDead-Git · 2023-02-26T02:52:52Z

Absolutely!

This reverts commit a872265.

- See labrocadabro#19 (review)

7MinutesDead-Git · 2023-02-26T04:11:06Z

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.

labrocadabro

It looks fantastic! There's just a couple of other issues:

it needs to be hidden when the user isn't logged in.
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.
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

I believe it just requires having two copies of the text and updating the clip-path on the
upper one dynamically, but if it turns out to be too much trouble, don't worry about it.

…value (...) - This allows for use of progress bar anywhere on the app. - The server will render the current completed lessonProgress count as inner meter's initial text.

- Pass in completed count to progress bar mixin

- Pass in completed and total lesson counts to progress bar mixin

7MinutesDead-Git · 2023-02-26T08:37:34Z

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.

7MinutesDead-Git · 2023-02-26T08:57:05Z

src/controllers/pages.js

@@ -23,7 +23,13 @@ export const dashboard = async (req, res) => {
 		currentLesson = await Lesson.findById(req.user.currentClass);
 		currentLesson = await getLessonProgress(req.user.id, currentLesson);
 	}
-	res.render("dashboard", { lesson: currentLesson });
+
+	const totalCount = await LessonProgress.countDocuments();


For totalCount, if traffic to your site ever starts really picking up, it may then be best to do this query only once on server startup, and keep its value cached in memory for subsequent requests for all users as it's unlikely the total lessons will change often (if ever). For now though this works for simplicity's sake.

labrocadabro · 2023-02-26T10:23:15Z

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:

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.
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:

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.
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.
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. :)

7MinutesDead-Git added 7 commits February 15, 2023 15:24

Removed duplicate semicolon

aa7da09

Simplified boolean expression

734316f

Merge branch 'main' of https://github.com/7MinutesDead-Git/communityt…

0461337

…aught into progress-meter

Add progressBar mixin to allLessons page

0dbaab1

Add initial progress bar styling

a872265

Add progressBar client-side logic

bc75418

Create basic progress bar mixin

1f7a06a

labrocadabro requested changes Feb 26, 2023

View reviewed changes

7MinutesDead-Git added 4 commits February 25, 2023 20:58

Revert "Add initial progress bar styling"

af86276

This reverts commit a872265.

Convert progressBar pug classes to tailwind (...)

2b7c349

- See labrocadabro#19 (review)

Refactor meter selector and variable name to be more specific

6613874

Add tailwind transition styling to progress meter (...)

ecad540

- See labrocadabro#19 (review)

7MinutesDead-Git requested a review from labrocadabro February 26, 2023 04:12

labrocadabro requested changes Feb 26, 2023

View reviewed changes

7MinutesDead-Git added 6 commits February 26, 2023 02:22

Add conditional render depending on presence of totalCount from server

fd0251a

Refactor initial completed count to be based on server-side rendered …

f51252f

…value (...) - This allows for use of progress bar anywhere on the app. - The server will render the current completed lessonProgress count as inner meter's initial text.

Include total and completed lesson counts in dashboard render

f6cba91

Include completed lesson count in allLessons render

12863e0

Render progress bar conditionally when logged in (...)

a03dcb9

- Pass in completed count to progress bar mixin

Render progress bar conditionally when logged in (...)

cd69c35

- Pass in completed and total lesson counts to progress bar mixin

7MinutesDead-Git requested a review from labrocadabro February 26, 2023 08:37

7MinutesDead-Git commented Feb 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add progress meter #19

Add progress meter #19

7MinutesDead-Git commented Feb 16, 2023

labrocadabro left a comment

7MinutesDead-Git commented Feb 26, 2023

7MinutesDead-Git commented Feb 26, 2023 •

edited

Loading

labrocadabro left a comment

7MinutesDead-Git commented Feb 26, 2023

7MinutesDead-Git Feb 26, 2023 •

edited

Loading

labrocadabro commented Feb 26, 2023

Add progress meter #19

Are you sure you want to change the base?

Add progress meter #19

Conversation

7MinutesDead-Git commented Feb 16, 2023

labrocadabro left a comment

Choose a reason for hiding this comment

7MinutesDead-Git commented Feb 26, 2023

7MinutesDead-Git commented Feb 26, 2023 • edited Loading

labrocadabro left a comment

Choose a reason for hiding this comment

7MinutesDead-Git commented Feb 26, 2023

7MinutesDead-Git Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

labrocadabro commented Feb 26, 2023

7MinutesDead-Git commented Feb 26, 2023 •

edited

Loading

7MinutesDead-Git Feb 26, 2023 •

edited

Loading