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.

Sign up for GitHub

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

Jump to bottom

yield setHook function #186

Merged
merged 4 commits into from
Apr 9, 2024
Merged

yield setHook function #186

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88a65d7
yield setHook function
miguelcobain Apr 6, 2024
a5be92e
Add a changeset
ynotdraw Apr 6, 2024
0a7524f
fix typecheck
miguelcobain Apr 6, 2024
6bdf51b
add setHook documentation
miguelcobain Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ember-velcro/src/components/velcro/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{{yield (hash
hook=this.velcroHook
setHook=this.setHook
loop=(if this.hook (modifier this.velcroLoop
this.hook
flipOptions=@flipOptions
Expand Down
9 changes: 7 additions & 2 deletions ember-velcro/src/components/velcro/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ interface Signature {
default: [
velcro: {
hook: ModifierLike<HookSignature>;
setHook: (element: HTMLElement | SVGElement) => void;
loop: ModifierLike<{
Element: HTMLElement;
}>;
Expand All @@ -46,9 +47,13 @@ export default class Velcro extends Component<Signature> {

setVelcroData = (data: MiddlewareArguments) => (this.velcroData = data);

setHook = (element: HTMLElement | SVGElement) => {
this.hook = element;
};

velcroHook = modifier<HookSignature>(
(element) => {
this.hook = element;
(element: HTMLElement | SVGElement) => {
this.setHook(element);
},
{ eager: false }
);
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

},
"pnpm": {
"overrides": {
Expand Down
27 changes: 27 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions test-app/tests/integration/components/velcro-test.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { render } from '@ember/test-helpers';
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { modifier } from 'ember-modifier';

import { findElement, resetTestingContainerDimensions, styleFor } from '../velcro-test-helpers';

Expand Down Expand Up @@ -37,6 +38,35 @@ module('Integration | Component | velcro', function (hooks) {
});
});

test('it renders with setHook', async function (assert) {
let hookModifier = modifier((element, [setHook]) => {
setHook(element);
});


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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

{{velcro.data.rects.reference.height}}
</div>
<div id="loop" {{velcro.loop}} style="width: 200px; height: 400px">
{{velcro.data.rects.floating.width}}
{{velcro.data.rects.floating.height}}
</div>
</Velcro>
</template>);

assert.dom('#hook').hasText('200 40', 'reference element has expected dimensions');
assert.dom('#loop').hasText('200 400', 'floating element has expected dimensions');
assert.dom('#loop').hasAttribute('style');
assert.dom('#loop').hasStyle({
position: 'fixed',
top: '40px',
left: '0px',
});
});

module('@middleware', function () {
test('it yields the MiddlewareArguments', async function (assert) {
await render(<template>
Expand Down