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

Fix prop-* type in BaseShapeTagHelper #5094

Closed
wants to merge 1 commit into from
Closed

Fix prop-* type in BaseShapeTagHelper #5094

wants to merge 1 commit into from

Conversation

hishamco
Copy link
Member

Fixes #5090

@hishamco
Copy link
Member Author

@agriffard this is a straightforward fix, hope you merge ASAP

@sebastienros
Copy link
Member

Please don't merge, I'll explain later

@giannik
Copy link
Contributor

giannik commented Dec 20, 2019 via email

@hishamco
Copy link
Member Author

Please don't merge, I'll explain later

I label the PR with dontmerge, waiting for your explanation

@jtkech
Copy link
Member

jtkech commented Dec 20, 2019

Hmm, this let me think that in Liquid i think we don't support yet DictionaryAttributePrefix.

For infos here _properties was never used, in one of my PR i just removed it.

@hishamco
Copy link
Member Author

Hmm, this let me think that in Liquid i think we don't support yet DictionaryAttributePrefix.

I see, I don't know about this one

@sebastienros
Copy link
Member

If _properties is not used, and removing it doesn't break prop-foo then remove it.
If you think you change is fixing something, I'd like to know what you have tested and how it fixed the issue.

@jtkech
Copy link
Member

jtkech commented Dec 20, 2019

Yes, i tested it _properties is useless

And i have to look if DictionaryAttributePrefix is implemented in Liquid

@hishamco
Copy link
Member Author

Yes, i tested it _properties is useless

It's useless before the PR

If you think you change is fixing something, I'd like to know what you have tested and how it fixed the issue.

I'm just asking why the dictionary accepts Object before? I think the pattern similar to what ASP.NET Core has 'asp-route-*`

@ns8482e
Copy link
Contributor

ns8482e commented Dec 21, 2019

i think we don't support yet DictionaryAttributePrefix.

@jtkech for info, I managed to add support for it while working on #4955 (still WIP), so that I could write asp-route-returnUrl in admin liquid template.

@jtkech
Copy link
Member

jtkech commented Dec 21, 2019

@ns8482e cool 👍 you will have to share the code ;)

@ns8482e
Copy link
Contributor

ns8482e commented Dec 21, 2019

you will have to share the code ;)

Sure! Let me create separate PR for that so that it can be independently merged and don't have to wait for #4955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting prop-* for shape tag helper throws an error
5 participants