HTML and CSS rework #72

sergei-maertens · 2022-03-14T13:58:35Z

The goal is to be less invasive and easier to theme, while cleaning up the actual CSS rules needed to make this work.

Tested against:

Django 2.2
Django 3.2
Django 4.0
Django 3.2 with Maykin default-project template

TODO:

Mobile styling (do we even want to bother? there's no hover state on mobile so we can't collapse/expand)
Make production CSS build instead of dev with sourcemaps
Regenerate package-lock.json with v2 format (use Node 16+)

Changes

Redid the CSS completely and properly isolated the dropdown-menu component
- Removed almost all the hardcoded position: fixed and top: $magicNumber rules, replaced with position: sticky or even relative in a number of places
- Properly leveraged flexbox to put the dropdown in the header while giving it 100% width
- Use the CSS vars from Django 3.2 and used default values from 3.2 for pre 3.2 Django versions.
- Added prefix to the CSS class names
Refactored the templates - now they extend from the django.contrib.admin templates, so they are now more compatible with different Django versions and less code for us to keep in sync with upstream
Dropped support for unsupported Django versions, only 2.2 LTS, 3.2 LTS and 4.0 are supported. It may still work on 3.0 and 3.1, but that's a problem for the library-user to solve now
Namespaced the inclusion template properly (Templates are not properly namespaced #69)
Added missing password change/done template overrides

Since we need to keep these templates in sync with Django's templates, it's important we can easily diff them and code-reformatting is detrimental for seeing actual meaningful changes.

For Django version pre 3.2, we define explicit fallback values equal to the default values of the vars in Django 3.2

Position it sticky so that scrolling keeps it in the page, but without absolute offsets.

* automatically gains the header bg color * inherits the z-index from the header container * removed the fixed/absolute positioning and hardcoded pixel offset * properly namespace the CSS class names (known to conflict with bootstrap before!)

* Use the CSS variables for color information * Lift styling rules to the item component and only override anchor styling where relevant * Fix the header to display the dropdown menus (overflow)

* Hover and active state use the default admin CSS vars but can be overridden easily * Moved CSS classes and styling to drop-item element rather than link itself

* The breadcrums are not position fixed/sticky anymore, however this should be easily overrideable in downstream projects.

codecov · 2022-03-14T15:12:30Z

Codecov Report

Merging #72 (44e697d) into master (7cc1e11) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   98.78%   98.78%           
=======================================
  Files          10       10           
  Lines         246      246           
  Branches       46       46           
=======================================
  Hits          243      243           
  Misses          2        2           
  Partials        1        1

Impacted Files	Coverage Δ
django_admin_index/admin.py	`100.00% <ø> (ø)`
django_admin_index/models.py	`100.00% <ø> (ø)`
django_admin_index/apps.py	`97.82% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cc1e11...44e697d. Read the comment docs.

Now they don't overflow beyond the viewport, but display a scrollbar. The bar itself is ugly, but at least usable.

These were originally copied from the admin/css/base.css file, but only a small subset are actually used by django-admin-index.

The default_auto_field should be specified. For the time being, we keep the Django 3.2 default value (AutoField rather than BigAutoField) and once only 3.2+ is supported, migrate to the new Django BigAutoFiel default.

* password change form * password change done

Django apps can inherit templates from other apps, so by putting admin-index before djang.contrib.admin, you can inherit the django admin default templates. This allows us to only override the bits that need to be overridden, making it easier to support multiple django versions and fixing/ applying the correct styling.

joeribekker · 2022-03-14T18:04:55Z

I'm marked as reviewer but still draft?

Regarding your TODO's. For mobile styling we can disable the menu.

sergei-maertens · 2022-03-14T21:42:50Z

yeah sorry, was a bit to eager with assigning a reviewer - there's a bit more tweaking to be done for easier overriding in default-project

Had so switch from older npm/node version

sergei-maertens · 2022-03-14T22:20:40Z

Ready for review now, and the accompanying PR for default-project is also prepared: https://bitbucket.org/maykinmedia/default-project/pull-requests/100

JostCrow · 2022-03-15T06:38:34Z

@sergei-maertens If you want to support mobile only with CSS, it is possible to do that with :focus and using tabindex="0" on the HTML element. That way it is also more accessible.

joeribekker · 2022-03-15T07:21:01Z

On second thought, it doesn't look that bad at the moment. With minimal effort, I think this actually works. Menus work since they are not links themselves.

So let's fix spacing and z index and go

sergei-maertens · 2022-03-15T08:58:54Z

Alright, I'll give it a stab with those suggestions. Would indeed be nice if we have basic functionality working and then leave room for improvement with another approach for a different PR

sergei-maertens · 2022-03-15T09:40:25Z

Good enough for now and tested on mobile

@JostCrow

Used tabindex=0 to make the elements focusable, thanks @JostCrow for the suggestion. The rest of the styling relies on the :focus pseudo-selector and some alignment tweaks on mobile.

svenvandescheur

The combinatin of sass and css variables confuses me a lot. Can you elaborate on the choices made?

svenvandescheur · 2022-03-15T09:47:54Z

.editorconfig

@@ -10,5 +10,8 @@ insert_final_newline = true
 charset = utf-8
 end_of_line = lf

+[*.scss]


Is there a reason to change this?

yeah, it's more in line with "big" mature projects like CRA. 2 spaces feels more natural than 4 tbh after working with both

scss/admin-index.scss

svenvandescheur · 2022-03-15T09:50:07Z

scss/_vars.scss

@@ -7,6 +7,10 @@ $color-secondary: var(--secondary, #417690);
 $color-accent: var(--accent, #f5dd5d);
 $color-primary-fg: var(--primary-fg, #fff);

+/* admin-index specific variables */
+// default of 10px is the padding-bottom default of the header
+$djai-dropdown-menu-spacing-top: var(--djai-dropdown-menu-spacing-top, 10px);


Is there a reason why we keep introducing variables while migrating to css variables? I think the combination is a bit confusing.

I suppose in this case we could set the default CSS variable values, but the order of stylesheet inclusion (admin-index is loaded in the body!) makes overriding in your own custom stylesheet harder than it should be. E.g.:

<head> <link href="/static/admin/css/base.css">  <link href="/static/admin_overrides.css">  </head> <body> ... <div class"djai-dropdown-menu"> <link href="/static/admin-index.css">  </div> </body>

This way, the CSS var is used if it is defined, otherwise the default value fallback is used. And to not have to repeat the fallback value everywhere, I've opted to wrap that in a sass variable.

scss/_vars.scss

joeribekker · 2022-03-15T10:31:55Z

functional good enough for now.

sergei-maertens added 10 commits March 14, 2022 11:37

💥 Drop support for EOL Django versions

de95829

🎨 Format template like upstream to make diffing easier

5f32126

Since we need to keep these templates in sync with Django's templates, it's important we can easily diff them and code-reformatting is detrimental for seeing actual meaningful changes.

🎨 Define sass variables pointing to the default admin CSS vars

9bb8b72

For Django version pre 3.2, we define explicit fallback values equal to the default values of the vars in Django 3.2

💄 #71 -- Use standard header flexbox layout

0ffe93f

Position it sticky so that scrolling keeps it in the page, but without absolute offsets.

💄 #71 -- style the dropdown menu dropdowns

5e13d18

* Use the CSS variables for color information * Lift styling rules to the item component and only override anchor styling where relevant * Fix the header to display the dropdown menus (overflow)

💄 #71 -- style the dropdown box using CSS vars

f5cbf7c

💄 #71 -- style dropdown menu items and links using CSS vars

32284c1

* Hover and active state use the default admin CSS vars but can be overridden easily * Moved CSS classes and styling to drop-item element rather than link itself

🔥 #71 -- delete style overrides we don't need

8c871c6

* The breadcrums are not position fixed/sticky anymore, however this should be easily overrideable in downstream projects.

🚚 Fixes #69 -- properly namespace inclusion template

0ec8e5b

sergei-maertens requested a review from dariomory March 14, 2022 13:58

sergei-maertens marked this pull request as draft March 14, 2022 15:46

sergei-maertens requested a review from joeribekker March 14, 2022 17:25

sergei-maertens and others added 5 commits March 14, 2022 18:44

💄 #71 -- handle dropdowns with a large amount of items

6b292c9

Now they don't overflow beyond the viewport, but display a scrollbar. The bar itself is ugly, but at least usable.

🔥 Delete unused variables

48cce16

These were originally copied from the admin/css/base.css file, but only a small subset are actually used by django-admin-index.

💄 #71 -- specify appropriate sidebar nav offset

e893c4a

💚 Update CI matrix — #71

b4a5a75

👽 Silence Django 3.2+ warning

1c77d1e

The default_auto_field should be specified. For the time being, we keep the Django 3.2 default value (AutoField rather than BigAutoField) and once only 3.2+ is supported, migrate to the new Django BigAutoFiel default.

sergei-maertens force-pushed the feature/71-markup-and-css-rework branch from 3ac858c to 8fe0f43 Compare March 14, 2022 17:44

sergei-maertens added 2 commits March 14, 2022 18:51

💄 #71 -- fix styling on Django 2.2

9714c04

🧑‍💻 Support DjDT for local testing

f12debd

sergei-maertens force-pushed the feature/71-markup-and-css-rework branch from 8fe0f43 to b2bbeda Compare March 14, 2022 17:51

sergei-maertens added 5 commits March 14, 2022 18:54

🔧 Fix isort config to be black compatible

72e3a61

🍱 Add missing template overrides to add the admin-index menu

8312cd2

* password change form * password change done

🔥 Delete unused CSS/sass

9a2136d

📝 #71 -- Update changelog with breaking changes

019d8eb

sergei-maertens force-pushed the feature/71-markup-and-css-rework branch from b2bbeda to 019d8eb Compare March 14, 2022 17:54

sergei-maertens added 5 commits March 14, 2022 22:54

💄 #71 -- defined some extra variables for theming purposes

aa18456

📝 #71 -- update README with theming instructions

737c407

📌 Pin dependencies in v2 format again

841fead

Had so switch from older npm/node version

🍱 #71 -- compile (minified) CSS

c8d2545

💄 #71 -- responsive styling

379668b

sergei-maertens marked this pull request as ready for review March 14, 2022 22:19

This was referenced Mar 14, 2022

Use "hamburger" for mobile menu #23

Open

#47 add support for django 3.2(LTS) #52

Closed

sergei-maertens requested a review from svenvandescheur March 14, 2022 22:28

📱 Implement functinal mobile design

44e697d

Used tabindex=0 to make the elements focusable, thanks @JostCrow for the suggestion. The rest of the styling relies on the :focus pseudo-selector and some alignment tweaks on mobile.

svenvandescheur reviewed Mar 15, 2022

View reviewed changes

sergei-maertens mentioned this pull request Mar 15, 2022

Issue/70 properly load stylesheets #74

Merged

sergei-maertens merged commit f85484a into master Mar 15, 2022

sergei-maertens deleted the feature/71-markup-and-css-rework branch March 15, 2022 10:40

sergei-maertens mentioned this pull request Mar 15, 2022

Rework CSS to support Django 3.2 properly #71

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML and CSS rework #72

HTML and CSS rework #72

sergei-maertens commented Mar 14, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading

joeribekker commented Mar 14, 2022

sergei-maertens commented Mar 14, 2022

sergei-maertens commented Mar 14, 2022

JostCrow commented Mar 15, 2022

joeribekker commented Mar 15, 2022

sergei-maertens commented Mar 15, 2022

sergei-maertens commented Mar 15, 2022

svenvandescheur left a comment

svenvandescheur Mar 15, 2022

sergei-maertens Mar 15, 2022

svenvandescheur Mar 15, 2022

sergei-maertens Mar 15, 2022 •

edited

Loading

joeribekker commented Mar 15, 2022

HTML and CSS rework #72

HTML and CSS rework #72

Conversation

sergei-maertens commented Mar 14, 2022 • edited Loading

codecov bot commented Mar 14, 2022 • edited Loading

Codecov Report

joeribekker commented Mar 14, 2022

sergei-maertens commented Mar 14, 2022

sergei-maertens commented Mar 14, 2022

JostCrow commented Mar 15, 2022

joeribekker commented Mar 15, 2022

sergei-maertens commented Mar 15, 2022

sergei-maertens commented Mar 15, 2022

svenvandescheur left a comment

Choose a reason for hiding this comment

svenvandescheur Mar 15, 2022

Choose a reason for hiding this comment

sergei-maertens Mar 15, 2022

Choose a reason for hiding this comment

svenvandescheur Mar 15, 2022

Choose a reason for hiding this comment

sergei-maertens Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

joeribekker commented Mar 15, 2022

sergei-maertens commented Mar 14, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading

sergei-maertens Mar 15, 2022 •

edited

Loading