Skip to content

Conversation

@msaraiva
Copy link
Contributor

@msaraiva msaraiva commented Feb 8, 2021

This PR adds a new function called attributes_escape/1 that will be used by the new template engine I'm working on. I'm not sure this function should be placed in the Tag module since neither tag nor content_tag use it. However, I believe they should use it for consistency. The problem is that, currently, the implementation sorts the attributes, which makes no sense for the new engine as it generates groups of attributes separately based on their types (static key/value, static key + dynamic value and dynamic key/value).

I can either move this function to someplace else or change the implementation of tag and content_tag to use it. The latter might break existing code if, for any reason, the order of the attributes was taken into account, for instance, in tests.

[32, "id", 61, 34, "the id", 34],
[32, "selected"]
]}
Copy link
Member

Choose a reason for hiding this comment

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

We should document that, opposite to tag and content_tag, the attributes are not sorted.

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Just one comment and we can ship it!

@msaraiva msaraiva marked this pull request as ready for review February 9, 2021 12:25
@josevalim josevalim merged commit 2d09867 into phoenixframework:master Feb 9, 2021
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@msaraiva
Copy link
Contributor Author

msaraiva commented Feb 9, 2021

@josevalim regarding accepting class: [active: path == "/"] as suggested by @wojtekmach in #276 (comment), I don't see why we shouldn't accept it. After all, the user can do stuff like ["user_#{id}": ...] on basically any function that accepts an atom or a keyword list. I don't see why we should treat this differently. Classes are, by nature, mostly not dynamic and when they are, the set of possible classes is still finite/predefined, not based on some infinite id. I actually never saw anyone using classes based on ids.

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.

2 participants