Harvard-ATG / hxat

Contains the currently-in-development project by HarvardX to bring the annotation tool currently living in the edX platform to a more accessible LTI implementation.
5 stars 1 forks source link

Refactored dashboard_view.html css to reflect navbar css from base.html #120

Closed Tanvez closed 2 years ago

Tanvez commented 3 years ago
dodget commented 2 years ago

Nice job @Tanvez , it looks like the PR resolves the issue. But I do have some concerns around the approach.

I think the immediate issue here is the clash of styling, which you clearly resolved. However, I think the bigger issue is the confusing layering of stylesheets. I would call that a cognitive load issue (when it is difficult to understand what is coming from where). I think reincorporating the appropriate styles from base.css in a style tag in the template increases the number of layers to the confusion cake. One approach I might try is to extract the styles included in the dashboard template (ie bootstrap_css) out to the base template, and placed above the base.css so the base css has the final say? I'm typing off the cuff here, but I think that would be a better resolution if it were possible.

Tanvez commented 2 years ago

Nice job @Tanvez , it looks like the PR resolves the issue. But I do have some concerns around the approach.

I think the immediate issue here is the clash of styling, which you clearly resolved. However, I think the bigger issue is the confusing layering of stylesheets. I would call that a cognitive load issue (when it is difficult to understand what is coming from where). I think reincorporating the appropriate styles from base.css in a style tag in the template increases the number of layers to the confusion cake. One approach I might try is to extract the styles included in the dashboard template (ie bootstrap_css) out to the base template, and placed above the base.css so the base css has the final say? I'm typing off the cuff here, but I think that would be a better resolution if it were possible.

ahh ok, Thanks Tylor! I will try this approach out.

arthurian commented 2 years ago

Totally agree with @dodget's concerns and suggested approach. Ideally, the hx_lti_initializer/base.html template would load the stylesheets such that:

  1. Vendor stylesheets, including bootstrap, are loaded first.
  2. Application-wide stylesheets such as base.css are loaded next.
  3. Per-template stylesheets are loaded last, either using {% block css_file %} to add them to the head or using {% content %} to add them directly to the body. In the case of the dashboard view, there would be nothing for this, so base.css would indeed be last.

I suspect you will run into some issues moving the bootstrap stylesheets into the hx_lti_initializer/base.html template since it seems like the admin_hub.html template solely relies on base.css for styling. There may be some conflicts, so you'll have to see if they can be resolved. In the long run, this is probably going to be more maintainable.

The good thing is that the blast radius of this change is limited to the admin hub and related admin screens for editing assignments, since the annotation screens (text, image, video) have a completely separate base template that doesn't depend on hx_lti_initializer/base.html at all. The annotation UIs inherit from hxiglighter_base.html which ultimately loads the CSS from Hxighlighter.