tl-its-umich-edu / canvas-app-explorer

A Web application that presents a list of Canvas external (LTI) tools with details. When integrated within Canvas, the user can search for specific LTI tool(s), and add or remove those tools from Canvas courses.
Apache License 2.0
4 stars 6 forks source link

Add customizable alt text for images (#144) #151

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

The PR aims to resolve #144. (Plasmic changes have already been made.)

jonespm commented 2 years ago

Tested this out and looks good

One suggestion is to make this change to admin.py

Then the image and the alt text are on the same line in the admin UI

image verses image

Easiest way I could see to do it.

I saw an app for adding alt text to any FileField and what we have seems simple enough for now since we've just got the 2 images.

diff --git a/backend/canvas_app_explorer/admin.py b/backend/canvas_app_explorer/admin.py
index 44f9761..73fa64b 100644
--- a/backend/canvas_app_explorer/admin.py
+++ b/backend/canvas_app_explorer/admin.py
@@ -4,7 +4,17 @@ from django.contrib import admin
 from backend.canvas_app_explorer.models import LtiTool, CanvasPlacement

 class LtiToolAdmin(admin.ModelAdmin):
-    pass
+    fields = (
+        'name',
+        ('logo_image','logo_image_alt_text'),
+        ('main_image','main_image_alt_text'),
+        'short_description',
+        'long_description',
+        'privacy_agreement',
+        'support_resources',
+        'canvas_placement'
+    )
+
 admin.site.register(LtiTool, LtiToolAdmin)

 class CanvasPlacementAdmin(admin.ModelAdmin):
ssciolla commented 2 years ago

One suggestion is to make this change to admin.py

Then the image and the alt text are on the same line in the admin UI

Sure, done, that seems like a nice change. https://github.com/tl-its-umich-edu/canvas-app-explorer/pull/151/commits/9e9c4317e767684d0343a18f352c5d4d2e26c8b8

Not totally convinced by the library, but maybe I'd have to try it out. Maybe if application was more about images, or we used template tags. It did occur to me that the data modeling is clunky (alt text and image path could be part of an image model, which could then be related to LtiTool M:1), but that's probably unnecessary, right?

ssciolla commented 2 years ago

Gonna merge for now, thanks for the review.