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

Add popover attributes #236

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Add popover attributes #236

merged 4 commits into from
Oct 30, 2024

Conversation

knpwrs
Copy link
Contributor

@knpwrs knpwrs commented Oct 26, 2024

This PR adds popover attributes as detailed here: https://developer.mozilla.org/en-US/docs/Web/API/Popover_API/Using

@markuswustenberg
Copy link
Member

Hi @knpwrs, thanks for this.

This is the second attribute in a short time that allows zero or one argument(s) (popover is the same as popover="auto"). I'm wondering whether we should start adding a third form of attribute in the html package, with varargs values, similar to how gomponents.Attr itself works. And then panic if there's more than one value. Thoughts?

@knpwrs
Copy link
Contributor Author

knpwrs commented Oct 28, 2024

I think that makes sense. Should this attribute and the others be moved somewhere for locality or should I just update this in place?

@markuswustenberg
Copy link
Member

@knpwrs I think in-place is fine. But it needs to go into a new test which:

  1. Tests that it works like a boolean attribute
  2. Tests that it works as a key-value attribute
  3. Tests that it panics with more than one value given

Might as well start a new table-driven test, I think more attributes like these will show up.

Are you up for that? Otherwise let me know. 😊

@knpwrs
Copy link
Contributor Author

knpwrs commented Oct 29, 2024

How does that look?

@markuswustenberg
Copy link
Member

Looks great! Should we add the popovertargetaction now we're at it? :D

@knpwrs
Copy link
Contributor Author

knpwrs commented Oct 30, 2024

Done!

@markuswustenberg
Copy link
Member

Thank you for your work! 😊

@markuswustenberg markuswustenberg merged commit 900ef87 into maragudk:main Oct 30, 2024
7 checks passed
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