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

Accept literals in attribute names #396

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

RedPhoenixQ
Copy link
Contributor

@RedPhoenixQ RedPhoenixQ commented Oct 19, 2023

The parser now accepts TokenTree::Literal as names for attributes.

To prevent the name_to_string function from showing and escaping the surrounding " of a string literal the string representation is trimmed first. This should not affect how other literals work which means that both "name"="true" and 123="true" would be valid.

This also addresses the PR request in #194

@fraschm1998
Copy link

Tried this, I seem to get:

"x-on:click"="toggle" type="button" aria-controls="mobile-menu" aria-expanded ="false" { ■ element body must be wrapped in braces = help: see https://github.com/lambda-fairy/maud/pull/137 for details

@RedPhoenixQ
Copy link
Contributor Author

Could you send more context like the macro where this element is created. This looks like and issue with missing {} for the element body or ; for a self closing element

@fraschm1998
Copy link

Could you send more context like the macro where this element is created. This looks like and issue with missing {} for the element body or ; for a self closing element

div."absolute inset-y-0 left-0 flex items-center sm:hidden" {                                                                                                                                                                                                                        
    1                                             // Mobile menu button                                                                                                                                                                                                                                                            
💥  2                                             button."relative inline-flex items-center justify-center rounded-md p-2 text-gray-400 hover:bg-gray-100 hover:text-gray-500 focus:outline-none focus:ring-2 focus:ring-inset focus:ring-indigo-500" "@click"="toggle" type="button" aria-controls="mobile-menu" aria-expanded="fa      lse" {     ■ element body must be wrapped in braces      = help: see https://github.com/lambda-fairy/maud/pull/137 for details                    
    3                                                 span."absolute -inset-0.5" {}                                                                                                                                                                                                                  
    4                                                 span."sr-only" { "Open main menu" }                                                                                                                                                                                                            
    5                                                 // Icon when menu is closed.                                                                                                                                                                                                                   
    6                                                 @for (item, path) in [("block", "M3.75 6.75h16.5M3.75 12h16.5m-16.5 5.25h16.5"), ("hidden", "M6 18L18 6M6 6l12 12")] {                                                                                                                         
    7                                                     svg.{ (item) " h-6 w-6" } fill="none" viewBox="0 0 24 24" stroke-width="1.5" stroke="currentColor" aria-hidden="true" {                                                                                                                                                  
    8                                                         path stroke-linecap="round" stroke-linejoin="round" d=(path) {}                                                                                                                                                                                                      
    9                                                     }                                                                                                                                                                                                                                                                        
   10                                                 }                                                                                                                                                                                                                                                                            
   11                                             }                                                                                                                                                                                                                                                                                
   12                                         } 

@RedPhoenixQ
Copy link
Contributor Author

I'm unable to reproduce the issue when copying the elements you sent. On the code that includes this PR it compiles for me and seemingly give the correct html.

This is the html string your code produces:
"<div class=\"absolute inset-y-0 left-0 flex items-center sm:hidden\"><button class=\"relative inline-flex items-center justify-center rounded-md p-2 text-gray-400 hover:bg-gray-100 hover:text-gray-500 focus:outline-none focus:ring-2 focus:ring-inset focus:ring-indigo-500\" @click=\"toggle\" type=\"button\" aria-controls=\"mobile-menu\" aria-expanded=\"false\"><span class=\"absolute -inset-0.5\"></span><span class=\"sr-only\">Open main menu</span><svg class=\"block h-6 w-6\" fill=\"none\" viewBox=\"0 0 24 24\" stroke-width=\"1.5\" stroke=\"currentColor\" aria-hidden=\"true\"><path stroke-linecap=\"round\" stroke-linejoin=\"round\" d=\"M3.75 6.75h16.5M3.75 12h16.5m-16.5 5.25h16.5\"></path></svg><svg class=\"hidden h-6 w-6\" fill=\"none\" viewBox=\"0 0 24 24\" stroke-width=\"1.5\" stroke=\"currentColor\" aria-hidden=\"true\"><path stroke-linecap=\"round\" stroke-linejoin=\"round\" d=\"M6 18L18 6M6 6l12 12\"></path></svg></button></div><h1 class=\"pinkie-123 asdf asdf asdf asdf asdf\">Pinkie Pie<button class=\"asdf\">test</button></h1>"

@fraschm1998
Copy link

Nvm. Seems to be working now. I think I may have just forgotten to clear the build cache.

Thanks!

Copy link
Owner

@lambda-fairy lambda-fairy left a comment

Choose a reason for hiding this comment

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

Thanks @RedPhoenixQ, looks great so far!

Can you please add a changelog entry as well?

maud_macros/src/ast.rs Outdated Show resolved Hide resolved
@lambda-fairy lambda-fairy removed their request for review November 6, 2023 05:29
Copy link
Owner

@lambda-fairy lambda-fairy left a comment

Choose a reason for hiding this comment

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

Thanks @RedPhoenixQ!

Let's merge what we have now. We can address my other comment in a follow-up PR.

html! { this b"byte_string"="false" 123="123" 2.5 true 'a'="a" b'b'="b" of-course {} };
assert_eq!(
result.into_string(),
r#"<this byte_string="false" 123="123" 2.5 true a="a" b="b" of-course></this>"#
Copy link
Owner

Choose a reason for hiding this comment

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

I think string literals, and maybe integers, are enough.

Floating point numbers overlap with class shorthand which also uses .. There's no use case for a single character. And no other part of Maud accepts byte strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lambda-fairy, addressing this issue, to keep ast::name_to_string() as an infallible api (always returning String without errors) I handled all the cases since I didn't see any issue with also handling ByteStr and others so they produce the expected internal string. Current implementation should always produce valid html.

The class shorthand shouldn't be broken since classes starting with numbers are disallowed in CSS so all classes before should still be possible. Also the only time a class starting with a number wouldn't be interpreted as a class would be 123.123 (space followed by numbers with dot and no spaces), but 123 .123, anything.123 and .classname123.123 would still parse as a class shorthand for the class 123.

Copy link
Owner

Choose a reason for hiding this comment

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

Having the interpretation change just by adding a space is the confusing part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#398 fixes this issue by running the existing literal() function in the parser when encountering a literal, reusing existing error behaviour. I've added Int as an accepted literal, but can revoke that change if you decide that Int shouldn't be allowed.

@lambda-fairy lambda-fairy merged commit 8c47208 into lambda-fairy:main Nov 14, 2023
4 checks passed
@RedPhoenixQ
Copy link
Contributor Author

I found a bug. Literals are now parsed and the try_name() function doesn't see literals, leading to code like html! { div test-123="true" {} } to be generated as <div test- 123="true"></div>

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.

4 participants