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

Improve HTML attribute support in client resource manager #5244

Merged
merged 7 commits into from
Aug 31, 2022

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Aug 31, 2022

Summary

This PR improves on #5242.

  • Use dictionaries of attributes directly instead of requiring them to be serialized and deserialized as strings
  • Add HTML attributes overloads for CSS
  • Clean up some comments, typos, and other code

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

This is awesome, even cleaner.
I used empty dictionaries so it does not get confused and hit the wrong overload (multiple strings passing nulls), never thought about using the parameter name to resolve that ambiguity. Great idea, and love the ternaries for empty strings, much much simpler and easier to understand!

@valadas
Copy link
Contributor

valadas commented Aug 31, 2022

@bdukes you think the build failure is a fluke

@bdukes
Copy link
Contributor Author

bdukes commented Aug 31, 2022

Yeah, I've re-run the build

Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

@david-poindexter david-poindexter merged commit 957fed8 into dnnsoftware:release/9.11.0 Aug 31, 2022
@david-poindexter david-poindexter deleted the html-attributes branch August 31, 2022 18:33
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.

None yet

3 participants