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

yield setHook function #186

Merged
merged 4 commits into from
Apr 9, 2024
Merged

Conversation

miguelcobain
Copy link
Contributor

I faced a situation where I need to apply the hook modifier + some custom logic to the element.
The problem is that modifiers aren't composable in ember. So we can't create a custom modifier that applies another modifier.

For that reason, in this PR I'm yielding a setHook function, thus increasing the composability options for users.

Copy link

changeset-bot bot commented Apr 6, 2024

🦋 Changeset detected

Latest commit: 6bdf51b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ember-velcro Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -17,7 +17,8 @@
},
"devDependencies": {
"@changesets/changelog-github": "^0.5.0",
"@changesets/cli": "^2.26.0"
"@changesets/cli": "^2.26.0",
"@glint/core": "^1.3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to make vscode glint extension happy and correctly detect gts files in sub packages.

@ynotdraw
Copy link
Contributor

ynotdraw commented Apr 6, 2024

Thanks @miguelcobain! This is excellent.

@miguelcobain
Copy link
Contributor Author

@ynotdraw I pushed a fix for the typecheck error CI was reporting. Please approve workflow. Thanks.

await render(<template>
<Velcro as |velcro|>
<div id="hook" {{hookModifier velcro.setHook}} style="width: 200px; height: 40px">
{{velcro.data.rects.reference.width}}
Copy link
Contributor

@nicolechung nicolechung Apr 8, 2024

Choose a reason for hiding this comment

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

Suggestion: Can you update the readme as to when you'd want to use setHook? While the PR description is helpful, I can't quite see how another modifier would make use of this.

Also the test doesn't show much other than it renders.

Copy link
Contributor Author

@miguelcobain miguelcobain Apr 8, 2024

Choose a reason for hiding this comment

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

@nicolechung

Also the test doesn't show much other than it renders.

The test shows that it renders and that the content was correctly positioned and sized. Which means that the setHook works, just like the hook modifier.

While the PR description is helpful, I can't quite see how another modifier would make use of this.

It would be a contrived example, but I'll explain my use case: Imagine you're writing a dropdown component with ember-velcro. You want to yield a trigger modifier that does two things:

  1. applies the hook from ember-velcro
  2. attaches a click handler to toggle between the open/closed states

Without setHook, this would not be possible. With setHook however, we can pass that function to the modifier, and the modifier can call that function.

The dropdown component might look something like this:

let myModifier = modifier((element, [setHook, handler]) => {
  // call ember-velcro's setHook
  setHook(element);
  
  // other custom logic
  element.addEventListener('click', handler);
  
  return () => {
    element.removeEventListener('click', handler);
  };
});

<template>
  <Velcro as |velcro|>
    {{yield (hash
      trigger=(modifier myModifier velcro.setHook onClick)
    )}}
  </Velcro>
</template>

This is needed because there's no way in ember (that I know of) to combine two existing modifiers into a single one. If there was, this PR wouldn't be needed.

Being this such an edge case, I'm not sure if we should document it with this detail. It might distract users from the main usage. Maybe adding this in a <details> tag in markdown?

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Apr 8, 2024

Choose a reason for hiding this comment

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

I wouldn't consider this an edge case -- in design systems, this sort of problem is IMMENSELY common -- it's a composibilitiy design challenge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolechung documentation was added

Copy link
Contributor

Choose a reason for hiding this comment

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

The test shows that it renders and that the content was correctly positioned and sized. Which means that the setHook works, just like the hook modifier.

I think (hope?) that should be enough for the tests. What I meant is that a user can't tell why they'd need to use setHook by looking at the test alone (apologize, was multi-tasking a bit and forget to edit my comment).

The documentation looks terrific, thanks for adding that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like when tests self-document the usage of something. This test passes the setHook into a custom modifier like it is intended to be used.
I'll gladly accept suggestions on how to improve the test.

1. sets an element as the "hook" for ember-velcro
2. attaches a click handler to toggle between the open/closed states

Without the yielded `setHook` function, this would not be possible. With `setHook` however, we can pass that function to the modifier, and the modifier can call that function with the element.
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ thank you! Very clear.

@ynotdraw ynotdraw requested a review from camskene April 9, 2024 12:38
@nicolechung nicolechung merged commit 42333b4 into CrowdStrike:main Apr 9, 2024
18 checks passed
@github-actions github-actions bot mentioned this pull request Apr 9, 2024
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.

None yet

4 participants