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

Added support for DictionaryAttributePrefix and ModelExplorer property in liquid #5099

Merged
merged 32 commits into from Apr 12, 2020

Conversation

ns8482e
Copy link
Contributor

@ns8482e ns8482e commented Dec 21, 2019

as per comments on thread
#5094 (comment)

Now, in liquid, defining following is possible,

{% block "form", action:"Edit", route-contentitemid: contentItem.ContentItemId, route-returnUrl:Request.Query["ReturnUrl"],  method:"post", enctype:"multipart/form-data" %}

{%endblock%}

And

{% form action:"Edit", route-contentitemid: contentItem.ContentItemId, route-returnUrl:Request.Query["ReturnUrl"],  method:"post", enctype:"multipart/form-data" %}

{%endform%}

Also Fixes #4426
Fixes #2740
/cc @jtkech

@jtkech
Copy link
Member

jtkech commented Dec 23, 2019

Looks good to me but i would need more time, i will review it asap.

There is another PR #4933 i'm working on and where i tried to improve the tag helper setters, see my comment. It is not incompatible with what you did here, but maybe better to wait for this PR in case it will be merged soon, then merge dev in your branch, then i will take the time to review this PR with you.

@ns8482e
Copy link
Contributor Author

ns8482e commented Dec 24, 2019

Added Support for ModelExplorer property, i.e now properties with asp-* having ModelExplorer datatype can be used in liquid. May fix #4426 and #2740

{% helper "label", for: "XXX"%}
{% helper "input", for: "XXX" %}
{% helper "span", validation_for:"XXX" %}

@ns8482e ns8482e changed the title Added support for DictionaryAttributePrefix in liquid Added support for DictionaryAttributePrefix and ModelExplorer property in liquid Dec 24, 2019
…ns8482e/liquid-idictonary-support

# Conflicts:
#	src/OrchardCore/OrchardCore.DisplayManagement.Liquid/TagHelpers/LiquidTagHelperActivator.cs
@jtkech
Copy link
Member

jtkech commented Feb 12, 2020

@ns8482e just did a little review only about formatting ;)

I assume you tried it, does it work as expected?

@ns8482e
Copy link
Contributor Author

ns8482e commented Feb 17, 2020

@jtkech yes, It works - However I'll add some sample in demo module and update the documentation too.

@ns8482e
Copy link
Contributor Author

ns8482e commented Feb 18, 2020

@jtkech Added CRUD sample that uses liquid template in OrchardCore.Demo

…ns8482e/liquid-idictonary-support

# Conflicts:
#	src/OrchardCore/OrchardCore.DisplayManagement.Liquid/LiquidViewTemplate.cs
#	src/OrchardCore/OrchardCore.DisplayManagement.Liquid/TagHelpers/LiquidTagHelperActivator.cs
@jtkech
Copy link
Member

jtkech commented Feb 18, 2020

@ns8482e okay cool ;)

I will take a look tomorrow

@ns8482e
Copy link
Contributor Author

ns8482e commented Feb 19, 2020

@jtkech @agriffard @Skrypt Updated document for liquid

@ns8482e
Copy link
Contributor Author

ns8482e commented Mar 21, 2020

@ns8482e

I did a review

Could you merge the last dev in your branch and then redo some tests as we have also updated LiquidTagHelperActivator through another PR that has been merged.

Then i will approve this PR

Works - No error

@jtkech
Copy link
Member

jtkech commented Mar 22, 2020

@ns8482e

I'm working on it and i did some updates that i will commit tomorrow as i have to use the web interface file by file (would be good to add my github user to your repo if not already done).

Here some comments on the main changes

  • When storing the TodoModel : ShapeViewModel you are saving in the database all the shape properties, so i separated things with a TodoModel and a TodoViewModel .

  • For the dictionary names and keys i don't use anymore the ToPascal conversions so i reverted the change in the related extension, and now we only check for a last underscore. So it is now as other liquid attributes that are intended to use underscores e.g route_returnurl.

  • So now we call LastIndexOf('_') once in place of LastIndexOfAny() 4 times without having to create a new char[] { '-', '_' } (that could have been a static field).

I will commit them tomorrow so that you will be able to see them more in details and do a review.

@ns8482e
Copy link
Contributor Author

ns8482e commented Mar 23, 2020

@jtkech you removed LastIndexOfAny so I guess route-todoid will not longer work as now it’s only looking for _ so route_todoid is only valid.

You may need to update docs for the same

@jtkech
Copy link
Member

jtkech commented Mar 23, 2020

@ns8482e

Yes for perf reasons and it simplified the code, i already updated the doc accordingly e.g to use route_returnurl, and i did again all the tests.

@jtkech
Copy link
Member

jtkech commented Mar 23, 2020

Forgot to say that i changed the Id property to a TodoId property because Id is a special property that is checked on yessql side and that somewhere may be related to the Id column.

Also having an identifier relying on the position of the list may create conflicts after removing / adding items. So i used an IIdgenerator, the same that we use e.g for ContentItem.

{% endform %}
```

In following example, `route-todoid` adds `Model.TodoId` to hyperlink.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

route_todoid

In following example, `route-todoid` adds `Model.TodoId` to hyperlink.

```liquid
{% a action: "Delete" , controller: "Todo", class: "btn btn-danger", route-todoid: Model.TodoId %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

route_todoid


```liquid
{% a action: "Delete" , controller: "Todo", class: "btn btn-danger", route-todoid: Model.TodoId %}
{% a action: "Delete" , controller: "Todo", class: "btn btn-danger", route_todoid: Model.TodoId %}
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

@ns8482e
Copy link
Contributor Author

ns8482e commented Mar 29, 2020

@jtkech Any other suggestions or concerns for this PR ?

@jtkech
Copy link
Member

jtkech commented Mar 30, 2020

@ns8482e

Any other suggestions or concerns for this PR ?

LGTM but because LiquidTagHelperActivator is a sensitive array about performance, i was waiting for a feedback from @sebastienros. That said, because of this line of code we check if we already have a setter before doing extra work, so i think that there is not so much impact on the performance.

@jtkech jtkech merged commit 5ef947a into OrchardCMS:dev Apr 12, 2020
scleaver pushed a commit to kast-cloud/OrchardCore that referenced this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forms: Add form controls tag helpers Create Liquid Tag for Validations, Notifications
5 participants