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

feat(popover): add support for popover target element #70

Merged
merged 35 commits into from
Feb 3, 2023

Conversation

yinonov
Copy link
Contributor

@yinonov yinonov commented Jan 27, 2023

Closes #66

Note that this PR:

  • Refactored popover triggers/invokers handling
  • Had to deviate and extend from the spec exact flow due to the lack of properties reference (e.g.

If node has a popovertoggletarget attribute, then set idref to the value of node's popovertoggletarget attribute

doesn't mentioned property check for popoverToggleTargetElement

  • introduced another triggers.ts file to extract payload of code from the main popover.ts
  • popover.ts is still cumbersome and already hard to read
  • should organize code more neatly
  • should write more tests
  • remove comments

@yinonov yinonov marked this pull request as ready for review January 28, 2023 19:40
@yinonov
Copy link
Contributor Author

yinonov commented Jan 29, 2023

@keithamus @jgerigmeyer
please review, as far as functionality. code runs as expected.
I'm yet sure about the style of code, it seem to bloat rapidly. WDYT?

src/popover.ts Outdated Show resolved Hide resolved
src/triggers.ts Outdated Show resolved Hide resolved
@yinonov
Copy link
Contributor Author

yinonov commented Jan 30, 2023

Copy link
Collaborator

@keithamus keithamus 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 looking close to shippable IMO. A couple of points though:

src/popover.ts Outdated Show resolved Hide resolved
src/popover.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@yinonov
Copy link
Contributor Author

yinonov commented Feb 1, 2023

I think we're done @keithamus

src/popover.ts Outdated Show resolved Hide resolved
src/popover.ts Outdated Show resolved Hide resolved
src/popover.ts Outdated Show resolved Hide resolved
yinonov and others added 5 commits February 2, 2023 12:52
Co-authored-by: Keith Cirkel <keithamus@users.noreply.github.com>
Co-authored-by: Keith Cirkel <keithamus@users.noreply.github.com>
Co-authored-by: Keith Cirkel <keithamus@users.noreply.github.com>
Copy link
Collaborator

@keithamus keithamus 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 looking great! Excellent work!

I'll leave @jgerigmeyer or @sanajaved7 to give a second review and merge, due to the size of the change. But this is 👌 in my view.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

One minor spelling suggestion, but this looks great! Thanks for the contribution. 🎉

src/popover.ts Outdated Show resolved Hide resolved
src/popover.ts Outdated Show resolved Hide resolved
yinonov and others added 2 commits February 3, 2023 07:47
Co-authored-by: Jonny Gerig Meyer <jonny@oddbird.net>
Co-authored-by: Jonny Gerig Meyer <jonny@oddbird.net>
@jgerigmeyer jgerigmeyer merged commit c176f35 into oddbird:main Feb 3, 2023
@jgerigmeyer
Copy link
Member

Published in v0.0.9 🚀

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.

Support target element reference
4 participants