-
Notifications
You must be signed in to change notification settings - Fork 2
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
Release 3.2 #105
Release 3.2 #105
Conversation
# Conflicts: # package.json # src/cdh/core/static/cdh.core/css/bootstrap.css # src/cdh/core/static/cdh.core/css/bootstrap.css.map # yarn.lock
That one _can_ handle proxy objects, such as lazy strings
…m-fields feature: fake form field for headers etc
…adme Docs/update readme
# Conflicts: # src/cdh/core/forms.py
Feature/uu list
feat: added Django 5 support
…rch-widget Feature/js search widget
BREAKING CHANGE: Drops support for 4.0 and lower
fed-auth is actually 4.1 now
use window.jQuery instead of window.$ to avoid clashes
Based upon the old cdh.core.mail code, but with refactored template-code and non-blocking errors. (Including new logging).
@miggol I've added you as a reviewer as a 'passing the buck' kind of thing. Actually reviewing is probably too much of a chore ;) The description is the draft release notes, which I always write as part of the staging -> main PR. (Mostly because the diff a PR gives me is a good way to write those) |
Alright, I'm starting to look at this today. As you say, won't be quality review. But it's a good reason to dive into the codebase and as good a place as any for questions. Congratulations on leaving such a legacy! Bit daunting, mind you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I went through most of this once and learned a lot! Nothing to really comment, just some questions.
I didn't go through cdh.mail or the vue-lib side of uu-list thoroughly, I'll get to figuring those out when I need them.
@@ -14,7 +14,7 @@ jobs: | |||
strategy: | |||
max-parallel: 2 | |||
matrix: | |||
python-version: ['3.9', '3.10'] | |||
python-version: ['3.11'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason tests don't all suported Python versions named in README.md?
I can imagine it eats up lots of runner minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing is now done using Django 5, which does not support Python 3.9;
At some point I had multiple requirements.txt
per Django version, which would allow proper testing of all version combinations, but that was a lot of (annoying) upkeep.
"author": "Humanities IT Portal development", | ||
"license": "Apache-2.0", | ||
"scripts": { | ||
"dev": "vite dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give me a quick rundown of how you develop a component that uses whole stack at once? Is it just running the Django test project with both Vite and sass watching for changes in the background?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sort off. Writing a Vue component just involves running yarn dev
(and the dev-app), and a lot of 'full-refreshing'.
The dev
command starts a watcher that compiles everything configured in the vite.config.ts
(in this case src/index.ts
, which is just a small set of imports/exports) into the configured output dir (the static-dir of the django app).
Any component or imported (S)CSS is compiled by Vite as well, so no extra command needed for that.
However, that's only for the Vue components. If you want to build any of the 'root stylesheets' shipped in the library (read: the full UU-bootstrap project + django specific additions), you'll need the build-css
, watch-css
and build-fedauth-css
commands in the root package.json
.
(The last one is a very minimal build of the UU-Bootstrap with only a small subset of the whole thing, to 1) have an independent build for the fed-auth app and 2) keep that build as small as possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is building an IIFE required or an optimization? Is IIFE a conscious choice over the other formats suggested by Vite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a conscious choice; the other options all build (relatively) complicated modules, which are a pain to load properly. (Ben has it working, but he compiles a full app/uses Vue in a global scope, we are interested in mounting a single component).
Basically, IIFE will compile everything into a single non-module file, which can be easily loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Django side of UU-List is looking super clean!
vV\|/vV|`-'\ ,---\ | \Vv\hjwVv\//v | ||
_) ) `. \ / | ||
(__/ ) ) | ||
(_/ | ||
Miauw! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:・゚✧(=✪ ᆺ ✪=);:・゚✧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just scrolling through this gives me secondhand trauma
If this is vanillajs-datepicker, it's just in here to not require a build step in projects that use DSC, right? Or have you made modifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a copy of the JS files shipped with the NPM package (node_modules/vanillajs-datepicker/dist/js/datepicker-full.js
), no edits. It's copied here to avoid having a build-step, yes. (AFAIK there is not a nice way to even do that in this specific case).
The needed CSS is custom/compiled in-project however;
See build-datepicker-css
in package.json
and https://github.com/CentreForDigitalHumanities/django-shared-core/blob/1397a7ea62828ce0e8378dcdcc0f01492b986074/assets/scss/vanillajs-datepicker.scss
That's needed because UU-Bootstrap != Bootstrap, so we cannot use the shipped CSS. (That, and this library does not actually build their CSS properly, but that's a different rant).
By compiling it ourselves we can make use of the bootstrap configuration shipped with UU-Bootstrap, which will make sure the right values are used. (The right colors, border roundiness, borders, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The separate rant being that they don't use the CSS variables Bootstrap provides, and instead only use SCSS variables. If they did the former, we probably could've re-used the shipped CSS. But alas, they don't, so we need to compile it with our own set of SCSS variables. It's not a 'bad' approach, it's a lot easier, just not a very 'bootstrap-y' way of doing things)
(This comment is mostly here for if someone else reads this comment through a google search or something, and thinks I'm being overly negative.)
Release 3.2
So, this is my last release as maintainer of this project. Sad noises :(
It's been interesting to see a project which was originally just a bad way to fix a non-problem turn out into a foundation for more than 10 applications. Talking about legacy :P
Anyway, so long, and thanks for all the fish
What's Changed
Features:
cdh.mail
; this new version is compatible with Django >=4.2cdh.core.mail
, so just update the imports/base templates. (Don't forget to addcdh.mail
toINSTALLED_APPS
)CDH_EMAIL_FAIL_SILENTLY
toFalse
in your settings, or provide thefail_silently=False
kwarg to the varioussend
methods/functions.BootstrapDateInput
,BootstrapSplitDateInput
Fixes:
ThreadLocalStorageMiddleware
SearchableSelectWidget
SearchableSelectWidget
now correctly takes the width it's given on the page.Deprecations
cdh.core
; use Form Media insteadcdh.core.mail
, usecdh.mail
instead. The old code is not usable in Django versions >= 4.2Docs:
Known problems
cdh.core.mail
does not work with Django >=4.2; Migrate tocdh.mail
for a working implementationFull Changelog: v3.1.0...v3.2.0