Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link

@madsmtm madsmtm commented Sep 10, 2018

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

@madsmtm
Copy link
Author

madsmtm commented Sep 10, 2018

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

@coveralls
Copy link

coveralls commented Sep 10, 2018

Pull Request Test Coverage Report for Build 37

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-1.7%) to 86.538%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flask_bootstrap/init.py 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
flask_bootstrap/init.py 1 86.54%
Totals Coverage Status
Change from base Build 28: -1.7%
Covered Lines: 45
Relevant Lines: 52

💛 - Coveralls

@madsmtm
Copy link
Author

madsmtm commented Sep 10, 2018

I'm still not really satisfied with how widgets, especially TableWidgets, are rendered inside ListWidgets, I'd like the label & help text to be in a top row if possible

@greyli
Copy link
Member

greyli commented Sep 12, 2018

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" #}
Copy link
Author

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?

Copy link
Member

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?

Copy link
Author

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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need Flask >= 1?

Copy link
Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right...

@greyli
Copy link
Member

greyli commented Sep 17, 2018

LGTM! Just go ahead to update the docs.

@greyli
Copy link
Member

greyli commented Sep 17, 2018

I've made the interface a lot more flexible, but and if you think its needed yet, I can make it backward compatible.

Does this PR break something?

@madsmtm
Copy link
Author

madsmtm commented Sep 17, 2018

Does this PR break something?

I've changed the interface of the render_field, render_form macros and removed the form_errors macro. I've also removed the bootstrap_is_hidden_field jinja function, and replaced it with the is_hidden test

@greyli
Copy link
Member

greyli commented Sep 17, 2018

IMO, it's a good idea to make it backward compatible.

@madsmtm
Copy link
Author

madsmtm commented Sep 17, 2018

I've tried documenting a little bit, but I'm unsure about the format. Should I:

  • Make a seperate file, where I put information about all the methods, and how to customize the form rendering
  • Only document the single render_form method

Or something in between those two?

@greyli
Copy link
Member

greyli commented Sep 17, 2018

I prefer to make a new file.

@@ -120,51 +84,20 @@ Example
API
~~~~

.. py:function:: quick_form(form,\
action="",\
method="post",\
Copy link
Member

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?

Copy link
Author

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

@greyli
Copy link
Member

greyli commented Sep 17, 2018

Can we just keep the old keyword arguments (i.e. action, method, extra_classes etc)? Both in the code and the docs. These arguments were commonly used when people rendering a form.

@madsmtm
Copy link
Author

madsmtm commented Sep 17, 2018

If you feel like we should do that, then yes, easily. Which ones do you want to keep precisely, action, method, role, enctype, id, novalidate and extra_classes?

@greyli
Copy link
Member

greyli commented Sep 17, 2018

@madsmtm We need to let user transfer easily from Flask—Bootstrap to this project, so I want to keep all of them.

@madsmtm
Copy link
Author

madsmtm commented Sep 17, 2018

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

@greyli
Copy link
Member

greyli commented Sep 18, 2018

OK, you are right, just update the docs will be enough.

@greyli greyli added enhancement New feature or request in process labels Nov 14, 2018
@greyli greyli added this to the 1.1 milestone Mar 13, 2019
@greyli greyli modified the milestones: 1.1, 2.0.0 Jun 13, 2021
@greyli greyli modified the milestones: 2.0.0, 3.0 Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request form in process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants