Fixes #35: Implemented Frontend Subscribe Form #36
Conversation
…uire certain methods to be implemented. Better to do this early when there's just one core backend in this app.
… any potential confusion in the future
There was a problem hiding this comment.
Sorry for the delay, there was a bit to cover in this. I like the approach to most of this, it makes sense.
The only thing that I'm tripping on is the subscribe form template + js. There's a lot of config for those templates and the js is a bit opinionated. We could simplify it a lot, still exposing the URLs + views but not having any included template. You could still import the form from birdsong but then it'd be up to you to set up the html however you needed.
birdsong/conf.py
Outdated
| BIRDSONG_SUBSCRIBE_FORM_FEEDBACK = getattr(settings, 'BIRDSONG_SUBSCRIBE_FORM_FEEDBACK', True) # bootstrap5 compatible validation feedback | ||
| BIRDSONG_SUBSCRIBE_FORM_NOVALIDATE = getattr(settings, 'BIRDSONG_SUBSCRIBE_FORM_NOVALIDATE', False) # disables built-in browser validation | ||
| BIRDSONG_SUBSCRIBE_FORM_ID = getattr(settings, 'BIRDSONG_SUBSCRIBE_FORM_ID', 'subscribe-form') # value of the form's html id attribute | ||
| BIRDSONG_SUBSCRIBE_FORM_BUTTON_LABEL = _(getattr(settings, 'BIRDSONG_SUBSCRIBE_FORM_BUTTON_LABEL', 'Subscribe')) # name of the form's submit button |
There was a problem hiding this comment.
considering the entire template is configurable, do we need to go into this level of configuration? Tend to lean towards the simpler the better, there's a lot of config options added here
There was a problem hiding this comment.
The idea behind having more flexibility via options was to allow adjustments without forcing devs to overload the template. Reasoning behind that was that as a dev I would rather keep using js driven by Birdsong for forward compatibility rather than override and diverge from Birdsong's core.
But I understand the desire to keep things simple so I've removed those above mentioned options.
Form feedback is now always on.
Built-in browser form validation is now always on (novalidate form attribute has been removed).
Form ID is now hardcoded to subscribe-form
Form button label is now hardcoded to Subscribe
Furthermore, I've also removed those template override related options and instead moved templates behind templates/birdsong directory. This allows for a more straightforward way to override subscribe form templates (one that is in line with how wagtail and other apps handle their templates).
| contact.is_active = True | ||
| contact.save() | ||
|
|
||
| return render( |
There was a problem hiding this comment.
I'm wondering if a redirect makes more sense here? The activate.html is pretty minimal, it wont be styled like the site
There was a problem hiding this comment.
I think that activation confirmation page is quite standard and (since it signals success to the end user) somewhat vital functionality of a subscribe form. Yes, it's not styled but neither is the unsubscribe.html page.
At the moment developers can implement their own page via your_app/templates/birdsong/activate.html. Overriding of which I have now streamlined in the code and described in the README file.
However, moving forward I think we can fix the underlying problem by implementing Birdsong's own base template (in a separate feature). That would simplify things and then we would only need to instruct developers to overload the base template (or teach birdsong how to integrate with their own), rather than keep asking them to overload individual html pages.
| const FORM_FEEDBACK_VALID = "valid-feedback" | ||
| const FORM_FEEDBACK_INVALID = "invalid-feedback" | ||
| const FORM_FEEDBACK_IS_VALID = "is-valid" | ||
| const FORM_FEEDBACK_IS_INVALID = "is-invalid" |
There was a problem hiding this comment.
same as above, remove any references to bootstrap specific classes
There was a problem hiding this comment.
I'm not sure how to handle this request. These classes are indeed used by bootstrap but in order for anyone to be able to format form feedback using CSS (with or without bootstrap) they need some classes to reference in their CSS files.
There are only two ways how I can remove these and that would be to either:
- change their values to break bootstrap but I'm unsure what other class names would be more appropriate
or - remove form feedback altogether (but I don't think you wanted me to do that)
All that these classes and related js code that uses them does is that it injects the following simple HTML feedback into the <form> tag
<div class="invalid-feedback">Invalid email address</div>
(and additionally it adds/removes is-valid / is-invalid class to/from the email field to expose it to css styling)
I think we should keep them, that way both bootstrap css would work out of the box but also others who don't use it would still be able to style the feedback as they see fit in their css without having to overload the birdsong_subscribe_form template tag. Win-win situation 😉
Simplified activation endpoint (no longer base64 encoding uuids)
…icts with other apps (same approach as wagtail's 'templates/wagtailcore' and other apps)
Described in README how to override Birdsong's subscribe form related templates
|
|
Now conventionally handling exceptions in subscribe_api (was previously trying to unnecessarily catch and trace which was messing up exception handling hooks such as sentry-sdk)
|
Sorted and ready for another review @seb-b |
|
I think at this stage this isn't really what we're looking for in this project, a lot of of this feels like things you should be implementing in your own project (templates/views etc), I'm still open to parts of this PR but as a whole it's too much |
Details in: #35