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

Fixed compatibility with Flask's built-in url type-converters #146

Merged
merged 10 commits into from
Sep 2, 2021
Merged

Fixed compatibility with Flask's built-in url type-converters #146

merged 10 commits into from
Sep 2, 2021

Conversation

caffeinatedMike
Copy link
Contributor

@caffeinatedMike caffeinatedMike commented Jul 23, 2021

Closes #136
Closes #145

With the addition of the model parameter to the render_table macro we can now utilize the previously private macro arg_url_for (previously _arg_url_for) to correctly grab values from each record as it's iterated over during table rendering. This allows users to utilize the built-in werkzeug converters with their url parameters.

Designed with backwards compatibility in mind. Users will still be able to pass in normal url_for calls as well. But, anything involving placeholders would be subject to the new tuple approach.

…odel to render_table. Added ability to supply url parameters to render_table by allowing passage of a list (first item is the route namespace, second item is a list of tuples mapping variables to placeholders representing fields present in the model)
… placeholders representing fields of a model.
@caffeinatedMike
Copy link
Contributor Author

I know I still need to update the docs, but just wanted to float this out there first for your inputs @greyli @michaelbukachi

@caffeinatedMike caffeinatedMike changed the title Fixed compatibility with Flask's built-in url_converters Fixed compatibility with Flask's built-in url type-converters Jul 23, 2021
@greyli
Copy link
Member

greyli commented Jul 24, 2021

Thanks for working on this! I'm preparing a presentation and don't have time to review it right now. I will take care of it at the beginning of the next month.

@greyli greyli added this to the 1.8.0 milestone Jul 24, 2021
@michaelbukachi
Copy link
Contributor

Nice work @caffeinatedMike! Don't you think it would be better to pass in a query instead of a model? I'm imagining scenarios where users might want to show specific subsets instead of everything in a table, based on certain filters.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Jul 26, 2021

Thanks @michaelbukachi! Just happy to be able to help/contribute. Good point! I'll make it a point to adjust it to accept a query, update the tests, and update the documentation once I get the chance over the next couple days. Is there anything specific I need to do to the documentation in order for things to render correctly? I noticed my addition of the custom_actions parameter didn't properly render the inline code (it just left the back-ticks).

@michaelbukachi
Copy link
Contributor

@caffeinatedMike Which file is not rendering properly?

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi The custom_actions part under parameters here

@greyli
Copy link
Member

greyli commented Aug 16, 2021

The custom_actions part under parameters here

The docs issue has been fixed in ab48a6c.

@caffeinatedMike
Copy link
Contributor Author

@greyli @michaelbukachi Changes made to accept query in place of model. Tests, docs, and changelog updated to reflect and all tests are passing.

flask_bootstrap/templates/bootstrap/table.html Outdated Show resolved Hide resolved
flask_bootstrap/templates/bootstrap/table.html Outdated Show resolved Hide resolved
docs/macros.rst Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
flask_bootstrap/templates/bootstrap/table.html Outdated Show resolved Hide resolved
@caffeinatedMike caffeinatedMike requested a review from greyli August 31, 2021 14:08
@greyli greyli merged commit 100923a into helloflask:master Sep 2, 2021
@greyli
Copy link
Member

greyli commented Sep 2, 2021

Merged, thanks!

@greyli greyli mentioned this pull request Sep 3, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants