-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
Improved form rendering #11
base: main
Are you sure you want to change the base?
Conversation
Since you can't easily override macros, I've wrapped them all in a block that you can then extend, the alternative would be this. There's also a block that you can extend if you need to provide rendering of custom form elements |
Pull Request Test Coverage Report for Build 37
💛 - Coveralls |
I'm still not really satisfied with how widgets, especially |
Thanks very much! I will review your code as soon as I have free time. |
{%- macro hidden_errors(field) -%} | ||
{%- block hidden_errors scoped -%} | ||
{# Doesn't actually work, has no element which makes it "display: block" #} |
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.
Also regarding this, do you know how the preffered way of displaying errors like this is in bootstrap? We can't use the normal <span class="invalid-feedback">
, since that's specific to form elements. Something like <div class="flash alert alert-danger">
perhaps?
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.
Use Alert will be good. Why adding a flash
class?
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.
I'm not really sure why I thought that, I probably just looked at some legacy code ;)
setup.py
Outdated
@@ -32,8 +32,11 @@ | |||
include_package_data=True, | |||
test_suite='test_bootstrap_flask', | |||
install_requires=[ | |||
'Flask' | |||
'Flask>=1' |
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.
Why need Flask >= 1?
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.
Because Flask
versions below v1.0.0
require Jinja2>=2.4
, while we need at least Jinja2>=2.8
(Or around there). Maybe it would be better to add an explicit requirement?
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.
All right...
LGTM! Just go ahead to update the docs. |
Does this PR break something? |
I've changed the interface of the |
IMO, it's a good idea to make it backward compatible. |
I've tried documenting a little bit, but I'm unsure about the format. Should I:
Or something in between those two? |
I prefer to make a new file. |
@@ -120,51 +84,20 @@ Example | |||
API | |||
~~~~ | |||
|
|||
.. py:function:: quick_form(form,\ | |||
action="",\ | |||
method="post",\ |
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.
Why these arguments were removed?
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.
I made them be part of kwargs
instead
Can we just keep the old keyword arguments (i.e. |
If you feel like we should do that, then yes, easily. Which ones do you want to keep precisely, |
@madsmtm We need to let user transfer easily from Flask—Bootstrap to this project, so I want to keep all of them. |
Well, but I don't see a reason to update the code, since after I made it backwards compatible, they can, they don't need to do anything. But if you want it added to the docs, I'll be happy to do so, I'm out of time for today though |
OK, you are right, just update the docs will be enough. |
Updated form rendering to properly use bootstrap v.4, it now also works with nested forms and custom form elements.
Basically, I just went to bootstrap/components/forms, and implemented the rendering based on the examples there.
I've made the interface a lot more flexible, but and if if you think its needed yet, I can make it backwards compatible
If you approve, I'll attempt to make some documentation