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

fix(parent/child): Add host argument to parent and child factories. #188

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

au-z
Copy link
Contributor

@au-z au-z commented Feb 18, 2022

Hi @smalluban!

Here's a totally unsolicited (but helpful) feature which you might find complementary to the existing parent/children factories...

For some complex components which reference parent and child elements based on element context, access to the host element is helpful. This can be done without a breaking change and creates some options for advanced component interactions.

For example, to get a parent which exposes a particular property name:

const MyChildComponent = define({
  tag: 'my-child-component',
  context: '__ctx__',
  contextProvider: parent((el, {context}) => el.hasOwnProperty(context)),
  // ...
})

Another use case:

const ChildElementCounter = define({
  tag: 'child-element-counter',
  childTag: '',
  children: children((el, {childTag}) => el.tag.toLowerCase() === childTag),
  // ...
})

Have a look and let me know what you think. Thanks! 👍

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 18, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cd2259d:

Sandbox Source
Hybrids web components playground Configuration

@coveralls
Copy link

coveralls commented Feb 18, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling cd2259d on auzmartist:parent-child-context into 92fc366 on hybridsjs:master.

Copy link
Contributor

@smalluban smalluban left a comment

Choose a reason for hiding this comment

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

Hi @auzmartist!

Thanks for your time in creating this PR. At first, i thought that it might be a not really useful feature, or it even might not work as you expect it, but after looking at this for a while, I can't make a reasonable argument against it.

I will merge the feature, but we need some changes:

…adding host argument to parent and child factories.
@au-z
Copy link
Contributor Author

au-z commented Feb 25, 2022

Alright, I've added a couple more tests and added to the API docs. Ready for re-review @smalluban. 👍

@au-z au-z requested a review from smalluban February 25, 2022 00:38
@smalluban smalluban merged commit 3d0dc3a into hybridsjs:master Feb 25, 2022
@smalluban
Copy link
Contributor

Nice work, thanks!

@au-z au-z deleted the parent-child-context branch February 25, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants