-
Notifications
You must be signed in to change notification settings - Fork 19
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
Implement filter
helper and use it to suppress reactive variables for non-widget fields
#128
Conversation
@robbiecarlton the biggest thing I'm worried about with this PR is that I kinda just cargo-culted the ownership/borrowing/consuming thing until |
@pdaoust A few things I'd like to do to get this merged
|
I'm going to go ahead and re-target this to |
19215a8
to
a87e3ba
Compare
Okay, this is now ready for review and can be merged when ready |
a87e3ba
to
9a7286d
Compare
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.
Better solution than what is proposed in #130 since we skip adding a widget_fields
field to the EntryDefinition
struct. I lean more towards this option as well
However, @ThetaSinner is right, some unit tests for the filter
helper will definitely be needed, with cases covering usage of the includeZero
optional parameter
@pdaoust kindly let me know if you are able to resolve the merge conflicts in this PR too
@ThetaSinner really sorry I missed all your messages from November; I don't have a good way of getting notifications from GitHub. @c12i thanks for the review; I'm really eager to get feedback on the aesthetics/idiomatic-ness of my code. Rust is very new to me, and I'd prefer iterators (coming from C# and JS) but don't always know how to do them in Rust. I'll merge from |
Co-authored-by: Collins Muriuki <[email protected]>
to match new organising pattern
Okay, I've tidied up all the code per your feedback @c12i and I've also merged in I'm pretty new to writing Rust, but I think I could come up with some tests that at the very least test the initial problem that this thing was trying to fix. How far do you think I should go -- wondering if it makes sense to test all possible falsey HBS values and a few representative truthy values, or if that's just overkill. |
Ah yes, should have mentioned this earlier, glad you made the change
No worries @pdaoust, we can test the happy path cases for now |
Co-authored-by: Collins Muriuki <[email protected]>
…olding into 2023-09-28-filter-helper
@c12i could you review again? I simplified some code -- realised that a couple suggestions above tackled one problem in two different ways and cancelled each other out :) I also wrote a few basic tests and discovered/fixed a bug. |
thanks for the changes @pdaoust, the implementation is much simpler now |
Implement `filter` helper and use it to suppress reactive variables for non-widget fields
#126 solved the problem of
EditFoo.svelte
templates creating trailing commas in the reactive variable declarations; e.g., for an entry typeFoo
with fieldsvisible_name
andhidden_birthdate
, the template would generate this line:But it was an ugly solution. This introduces a better solution via a new
filter
helper for Handlebars. You pass it a value containing a list of items and a predicate, and it gives back a list of filtered items. Then you can filter out non-widget fields and use HBS'last
helper to suppress the final comma. (I'm sure it has other uses too.)It just renders a very tiny Handlebars template that uses
{{#if}}
to check the predicate against each item. This lets you use Handlebars' full functionality in the predicate.