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

Custom tags don't support Hash #1418

Closed
confact opened this issue Feb 23, 2021 · 5 comments · Fixed by #1453
Closed

Custom tags don't support Hash #1418

confact opened this issue Feb 23, 2021 · 5 comments · Fixed by #1453
Assignees
Labels

Comments

@confact
Copy link
Contributor

confact commented Feb 23, 2021

Describe the bug
custom tags don't support Hash as params, but divs and others do. This makes it hard to make custom tags work with flexible data.

To Reproduce
make a normal Hash(String, String) called params with variables.
make a code like:

tag "turbo-frame", params do
  text "test"
end

If you make it with div like:

div params do
  text "test"
end

It will work.

Expected behavior
Have the same consistent behavior everywhere. Make custom tags work as standard tags.

Versions (please complete the following information):

  • Lucky version (check in shard.lock): 0.26
  • Crystal version (crystal --version): 0.36.1
  • OS: Linux
@confact confact added the bug label Feb 23, 2021
@jwoertink
Copy link
Member

Does it work if you do this?

tag "turbo-frame", options: params do
  text "test"
end

@confact
Copy link
Contributor Author

confact commented Feb 24, 2021

@jwoertink yea, that worked. ^^ - Is that supposed to work like that? Would nice that they work the same as the others or is it something that makes it harder?

@jwoertink
Copy link
Member

It works because the second argument is for the valueless args like if you were writing some Vuejs tag "turbo-frame", [:v_bind], and the third arg is the html opts. So this should also work

tag "turbo-frame", [] of Symbol, params do
  text "test"
end

Maybe we can use an outer variable for that? Or possible swap those, but not sure how much damage that might cause 🤔

@robacarp
Copy link
Contributor

@jwoertink does it make sense to provide an overload of the tag method which takes the params hash as the second parameter? Going too far down that road might make things confusing but maybe this isn't a step too far?

@jwoertink
Copy link
Member

Yeah. It looks like this current overload is to match the base ones https://github.com/luckyframework/lucky/blob/master/src/lucky/tags/base_tags.cr#L54 but we seem to be missing this variant https://github.com/luckyframework/lucky/blob/master/src/lucky/tags/base_tags.cr#L46 (without the attrs). I think if we add that in, then it should work.

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

Successfully merging a pull request may close this issue.

4 participants