preaction / Yancy

The Best Web Framework Deserves the Best Content Management System
http://preaction.me/yancy/
Other
54 stars 21 forks source link

Added skip_fields option to Yancy::Plugin::Form::Bootstrap4::form_for() #94

Closed pavelsr closed 4 years ago

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.01%) to 94.674% when pulling fbc5d50ae0e84da65f9ee662283c7a6f22016d10 on pavelsr:issue_93 into a5f7e325f75e7623e7d10c73bfc5379b772ad756 on preaction:master.

preaction commented 4 years ago

We should definitely do something like this, but would it be better to make this "fields to include" instead of "fields to remove"? I generally like the idea that things shouldn't be exposed to untrusted users without an explicit instruction, and the form plugin is meant for the untrusted users.

If we instead made a properties option to form_for which took an array reference of fields to show, we could make the default use the properties stash value set in the controller and then everything lines up: Setting the properties stash on a set route makes the form_for only display those fields. Getting a default from the stash also means less messing around with templates: Yancy could come with one template that simply displays a form and is configured entirely through the stash.

pavelsr commented 4 years ago

I think it would be more convenient if we will have both "fields to include" and "fields to remove" options. As for me, "fields to remove" option is fastest to implement.

Using "fields to include" and properties stash value could be profitable only if template, rendered by Yancy::Controller::Yancy::set() ( by default templates/yancy/set.html.ep ) contains modified/created data. As for me, I prefer to pass form data using JSON and I consider inconvenient to have one universal "conditional" template for different schemas (e.g. it's hard to customize it using jquery).

So, if we want to implement "fields to include" option we have to to create and add to repository also a template similar to table.html.ep but only for viewing one item from any schema.

Anyway, your decision. For me it's not principal question, to have "fields to include" or "fields to remove". It's critical to have any of this right now :)

preaction commented 4 years ago

Alright, I'll merge this and reverse the logic (you've already done most of the work). Thanks for the patch!