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

feat(canvas-lms): rewrite #1084

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

pynappo
Copy link

@pynappo pynappo commented Jul 18, 2024

(reopening because I didn't name my branch properly, apologies)

🔧 What does this fix? 🔧

  • Borders are themed to less standout colors
  • User content (i.e. text with user-added highlights/text color) will keep their original styles when needed
  • Course-specific colors are preserved
  • Links are themed
  • Buttons are themed and have proper hover color changes
  • Accent color is more prominent wherever appropriate
  • Doesn't have Solid bar on top of quizzes #368 although I haven't seen the issue with the old nor the new userstyle, maybe due to new canvas changes
  • School-specific brand colors are kept on the global nav sidebar (down to change but personally i think it's nicer this way)
  • etc.
Before/After on Catppuccin Mocha with Green accent color

(white borders on the screenshot are my browser not the userstyles)

Dashboard

Before
current dashboard

After
rewrite dashboard

Example Course Homepage

Before
current course homepage

After
rewrite course homepage

Example Quiz

Before
current quiz

After
rewrite quiz

Example Course Modules

Before
current modules

After
rewrite modules

I've themed the main pages that I've used as a student during my courses.

A lot of the canvas CSS is autogenerated but with stable suffixes/prefixes/children, hence the high use of attribute selectors and such. edit: apologies for the large line count increase, styling the site properly is a pain.

🗒 Checklist 🗒

  • I have read and followed Catppuccin's contributing guidelines.
  • I have updated the version appropriately in the ==UserStyle== header of the catppuccin.user.css file.

Other things to do:

  • Theme less-used pages (Canvas Studio, instructor-side UI, etc)
    • styled a few of the instructor-side pages and Canvas Studio, leaving the rest for later since students like me only use like 20 pages
  • Clean up the CSS (maybe for later)

@github-actions github-actions bot added the canvas-lms Canvas LMS label Jul 18, 2024
@pynappo pynappo marked this pull request as ready for review October 29, 2024 10:57
@pynappo pynappo requested a review from a team as a code owner October 29, 2024 10:57
uncenter

This comment was marked as outdated.

@uncenter

This comment was marked as outdated.

@uncenter
Copy link
Member

I was able to use the following CSS to theme all of those little and sometimes big spinners Canvas uses:

[class*="spinner__circleTrack"] {
    stroke: @surface1 !important;
}
[class*="spinner__circleSpin"] {
    stroke: @accent-color !important;
}

@uncenter uncenter added the waiting on author Note for staff that a re-review is not yet required label Nov 13, 2024
@uncenter
Copy link
Member

I'm going to add a "waiting-on-author" label - no pressure, just signals to maintainers at a glance that a review is not required yet. Please leave a comment once you believe you have implemented the suggested changes and fixed the unthemed areas :)

@pynappo
Copy link
Author

pynappo commented Nov 13, 2024

For the assignment-page/upload styles, i don't think canvas.instructure.com nor my school's instance has Enhanced Assignments enabled so I can't get those pages to show up on my end - if I could get some html snippets/css styles for those that'd be greatly appreciated.

For sidebar, I left that unstyled because I thought it would help with people who enroll at multiple colleges. Should I just style it according to accent-color?

@uncenter
Copy link
Member

For sidebar, I left that unstyled because I thought it would help with people who enroll at multiple colleges. Should I just style it according to accent-color?

I would say make it mantle?

@uncenter
Copy link
Member

Pushed some fixes. Not complete yet, and not in the right place lol. Trying to juggle some files with new documentation I plan to introduce to go along with some of the stuff in that commit. If you want you can organize the new enhanced assignments section stuff I added into the right place and I'll keep working on it wherever you move it to when I get back to it.

@uncenter

This comment was marked as outdated.

@pynappo pynappo requested a review from isabelroses as a code owner January 28, 2025 10:16
@pynappo
Copy link
Author

pynappo commented Jan 28, 2025

Oops didn't mean to request a review sorry

You can see -baseButton__content is part of the class that is targeted here. Searching this finds that you are already theming some of these types of buttons, though you are using autogenerated classnames in some places - please avoid that if possible so we can catch more of these buttons in our theming.

Hard to avoid, I think relying on at least some auto-generated class names and documenting where they may appear is the easier path for maintenance. canvas has way too many places that use auto-generated css. The main one targeted, css-1rpus1y-baseButton__content covers a majority of the neutral-colored buttons, i'll see if there's only a few more auto-generated ones that have similar levels of reuse.

@uncenter
Copy link
Member

relying on at least some auto-generated class names and documenting where they may appear is the easier path for maintenance

It'll actually cause you more trouble since every time Canvas edits that component or whatever, sometimes even every time they deploy, the CSS autogen classnames will update and won't select correctly anymore. We get this often with the Gmail userstyle and it is a real PITA. Only do it if you must I guess.

@uncenter
Copy link
Member

I'm going to do a squash merge of all your commits on this branch and then rebase it with main now. Is that okay?

@uncenter
Copy link
Member

If you could please setup/install https://git-lfs.com for your local userstyles fork, you should get a "change" to the preview.webp file you added in this PR, and then can you commit & push that change up to this branch?

@pynappo
Copy link
Author

pynappo commented Jan 29, 2025

I'm going to do a squash merge of all your commits on this branch and then rebase it with main now. Is that okay?

yep, go ahead!

We get this often with the Gmail userstyle and it is a real PITA. Only do it if you must I guess.

Yeah that makes sense. Later I can take some time to change to using mixins and more stable selectors to style each button.

@uncenter
Copy link
Member

Can you invite me as a collaborator to your userstyles fork? Having more annoying issues with Git refusing my pushes because I'm "uploading LFS objects" (I'm not).

@uncenter uncenter removed the waiting on author Note for staff that a re-review is not yet required label Jan 29, 2025
@uncenter
Copy link
Member

(None of this is your fault, just Git and Git LFS and GitHub not cooperating.)

@uncenter uncenter force-pushed the refactor/canvas-lms branch from 6d4f509 to 8fd9d3e Compare January 30, 2025 03:05
@uncenter uncenter changed the title refactor(canvas-lms): rewrite feat(canvas-lms): rewrite Jan 30, 2025
@uncenter
Copy link
Member

All the changes should be there, except the preview image (it needs to be retaken anyways). There are no more conflicts, but the linter is complaining about a few things - deno task lint:fix canvas-lms should autofix most/all of them for you.

@uncenter uncenter added the waiting on author Note for staff that a re-review is not yet required label Jan 31, 2025
@uncenter
Copy link
Member

uncenter commented Feb 1, 2025

When taking the preview can you try to make Canvas take up the whole screen? Looks like only the first half of the preview image actually has Canvas on it. Also if you haven't already see https://github.com/catppuccin/userstyles/blob/main/docs/tips-and-tricks/convert-images-to-webp.md for our recommendation on converting images to WebP (seeing some color quality loss in the current preview).

@uncenter uncenter removed the waiting on author Note for staff that a re-review is not yet required label Feb 1, 2025
@pynappo
Copy link
Author

pynappo commented Feb 1, 2025

i think the preview should be good now, lmk if you want me to remove the slightly low res png one of my classes has and i'll just retake it.

also, does catwalk handle the conversion from png to webp losslessly as well? or is converting the png screenshots to webp via command still preferred?

@uncenter
Copy link
Member

uncenter commented Feb 1, 2025

I personally convert before Catwalk-ing, I haven't exactly tested it though. It's also easier because I have a shell script png_to_webp to recursively convert to WebP, and Catwalk defaults to looking for <flavor>.webp files in the CWD - so all I need to do is png_to_webp && catwalk.

@uncenter
Copy link
Member

Woah this is looking really good. Nice work. I think I'm good to merge this, thought I'd mention this minor issue in the compose message modal of the inbox:

CleanShot 2025-02-13 at 08 33 47

I don't want to take up too much more of your time and energy, so if you don't have the chance to get to that soon I'll just merge today :)

@pynappo
Copy link
Author

pynappo commented Feb 18, 2025

Woah this is looking really good. Nice work. I think I'm good to merge this, thought I'd mention this minor issue in the compose message modal of the inbox:

[...]

I don't want to take up too much more of your time and energy, so if you don't have the chance to get to that soon I'll just merge today :)

I should be able to fix this later today, i'll lyk

@pynappo pynappo force-pushed the refactor/canvas-lms branch from c970a67 to ed781e0 Compare February 18, 2025 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
canvas-lms Canvas LMS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants