jamesjonesmath / canvancement

Enhancements to the Canvas LMS
ISC License
214 stars 95 forks source link

Add sort direction indicators; scroll to trigger loading full list #56

Open simonrob opened 10 months ago

simonrob commented 10 months ago

This PR has two changes:

I'm hoping the first change is pretty easy to include as it makes the interface clearer and easier to use. The second is up to you depending on whether you feel the approach is too hacky (or indeed not desired). Feel free to remove the changes to watchForAdditionalRows if that's the case.

jamesjonesmath commented 10 months ago

This request is a bit more involved. I'm at school right now and won't get a chance to look at it until later. I would need to do some testing before implementing parts of this.

I would need to make sure that the icons indicating direction are actually clickable and doesn't break other things.

Was there a reason you added your own instead of using the built-in icons for jquery tablesorter? Maybe it's quicker your way or you don't like their styling.

I would add the CSS through pure JavaScript rather than using the GM addStyle. There's just something comforting to me about having grant none. Makes it seem like I'm not trying to do anything terrible that isn't normally allowed.

I don't think I want it to scroll when the first page is loaded. When does it stop? If I have 500 students in a class, do I really want to pre-load all of the students? I'm probably not using the people page if I have 500 students because it's worthless in most cases. The filtering capability only allows filtering by role. What's up with that Canvas? Let me filter by section!

And what happens to your script if the user starts to move the page rather than waiting? Also, I would only want to scroll if there was a multiple of 50 and then I would likely want to remove the observer once the list was completely loaded. Since some of those lines are white (original), I must have matured in my programming skills since I wrote it and am now thinking of new things to do.

One of the things I dislike about Canvas now is their automatically jumping the page around when I do things. For example, I have a bunch of modules I want to move, but I have to scroll after every one because it jumps me to where the module ends up rather than leaving me on the next module (or previous if it was the last one).

Unexpected movements can cause accessibility issues. While aware of them, I don't have the resources like Canvas to program around them in all cases, so people using my scripts have to realize accessibility may break. But I want to test it to make sure it doesn't break something very badly before I do.

simonrob commented 10 months ago

Thanks for taking a look at this.

Re: icons: the inbuilt tablesorter ones require an extra stylesheet to be added that includes the icon images, but most of the styling is unnecessary (or would conflict with Canvas). They are implemented using the standard tablesorter approach, though, so are closely integrated with the existing sorting behaviour. The icons need to be a CSS class rather than added directly to the header elements, so I thought GM_addStyle was a cleaner way to do this. But a style element could be created using pure JavaScript if needed. The icons themselves are clickable, and don't break other things.

Re: scrolling: I totally agree with your Canvas criticisms around behaviours like this! Just to be clear, though, the scroll here is not actually perceptible by the user – it's 1px one way and then immediately 1px back, which just triggers the page's existing scroll tracking code (I did also try jq(window).trigger('scroll'); for a zero-movement approach, but this wasn't reliable).

It turns out that any amount of scroll movemement on the page is what causes the next set of students to be loaded, so only a tiny amount is needed. (As an aside: this is such a strangely-implemented approach – it must be rare for the entire list not to be loaded given this design, and all it really serves to do is make it painful to quickly get the full list). There's no problem if the user themselves scrolls around the page, and the script could easily set a maximum number of students to load automatically via this method.

Re: design: I took the approach that required the least changes, so didn't edit any of the existing observer behaviour. I agree the underlying script could be improved here, but also think it's not worth worrying too much about.

simonrob commented 9 months ago

I've just discovered that Canvas has inbuilt CSS styles that work well for this, so I've replaced them in this script, and also added the double indicator to the all courses sort script too.