-
Notifications
You must be signed in to change notification settings - Fork 187
feat(calculateInPlacePosition): implement verticalPosition "auto" #1016
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
base: master
Are you sure you want to change the base?
feat(calculateInPlacePosition): implement verticalPosition "auto" #1016
Conversation
28b4a45 to
095e8c9
Compare
|
@velrest Thanks for your PR! According to the Ember Basic Dropdown documentation, you need to pass the
Adding this feature will require a major version (which is already planned), since it changes the current behavior. Could you also add some tests to ensure that this change works correctly and does not introduce any regressions? |
|
@mkszepp I will change the commit message and add some tests on monday. Thanks for the link! |
095e8c9 to
f71c7bd
Compare
|
@mkszepp I'm actually not quite sure on how to write an integration test for this as it seems there is no test for |
|
Oh, interesting that we have no tests right now for this... 🙉 I will take a look in next days, how we can add useful tests for |
|
@mkszepp I tried to create a setup for this in my test commit but it didn't work. It seems like, the whole position calculation is never called it that rendering test, as the initial calculation is done outside the run loop? Also tried to emit a scroll event to force recalculation but that also didn't work. |
|
@velrest sry for my delayed answer... you can add into test<ExtendedTestContext>('It adds the proper class above to trigger and content when it receives `renderInPlace={{true}}` and @verticalPosition="auto"', async function (assert) {
assert.expect(2);
await render(hbs`
<div style="position: absolute; bottom: 0">
<BasicDropdown @renderInPlace={{true}} @verticalPosition="auto" as |dropdown|>
<dropdown.Trigger>Press me</dropdown.Trigger>
<dropdown.Content><h3>Content of the dropdown</h3></dropdown.Content>
</BasicDropdown>
</div>
`);
await click(
getRootNode(this.element).querySelector(
'.ember-basic-dropdown-trigger',
) as HTMLElement,
);
assert
.dom('.ember-basic-dropdown-trigger', getRootNode(this.element))
.hasClass(
'ember-basic-dropdown-trigger--above',
'The proper class has been added',
);
assert
.dom('.ember-basic-dropdown-content', getRootNode(this.element))
.hasClass(
'ember-basic-dropdown-content--above',
'The proper class has been added',
);
});
test<ExtendedTestContext>('It adds the proper class below to trigger and content when it receives `renderInPlace={{true}}` and @verticalPosition="auto"', async function (assert) {
assert.expect(2);
await render(hbs`
<div style="position: absolute; top: 0">
<BasicDropdown @renderInPlace={{true}} @verticalPosition="auto" as |dropdown|>
<dropdown.Trigger>Press me</dropdown.Trigger>
<dropdown.Content><h3>Content of the dropdown</h3></dropdown.Content>
</BasicDropdown>
</div>
`);
await click(
getRootNode(this.element).querySelector(
'.ember-basic-dropdown-trigger',
) as HTMLElement,
);
assert
.dom('.ember-basic-dropdown-trigger', getRootNode(this.element))
.hasClass(
'ember-basic-dropdown-trigger--below',
'The proper class has been added',
);
assert
.dom('.ember-basic-dropdown-content', getRootNode(this.element))
.hasClass(
'ember-basic-dropdown-content--below',
'The proper class has been added',
);
});Everything else, changed in test-app i think it's not needed and can be reverted |
| content: HTMLElement, | ||
| ) => GetViewDataResult; | ||
|
|
||
| export const getViewData: GetViewData = (trigger, content) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we don't need to export this... when somebody wants to override our default functions they should make everything custom, not only a part
| viewportBottom: number; | ||
| }; | ||
|
|
||
| export type GetViewData = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export type GetViewData = ( | |
| type GetViewData = ( |
| options: CalculatePositionOptions, | ||
| ) => CalculatePositionResult; | ||
|
|
||
| export type GetViewDataResult = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export type GetViewDataResult = { | |
| type GetViewDataResult = { |
|
|
||
| @action | ||
| reposition(): undefined | RepositionChanges { | ||
| debugger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugger should be removed
| content.getBoundingClientRect(); | ||
| const viewportWidth = document.body.clientWidth || window.innerWidth; | ||
| const viewportBottom = scroll.top + window.innerHeight; | ||
| debugger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debugger should be removed here
74b2836 to
9195fa9
Compare
9195fa9 to
1d4329f
Compare
|
@mkszepp I update the PR with your tests and Review suggestions. The test passes without my code changes and the code in the function |
The
ember-power-selectdocumentation mentions, that the argumentverticalPositionis by defaultautoand therefor the positioning will be automatically calcualted based on available space. This is correct for wormhole positioning but not if the argumentrenderInPlace={{true}}is used as this is not implemented here inember-basic-dropdown.I was not sure if this is a bug or rather a new feature in the scope of
ember-basic-dropdown.Reference
Reproduction: https://github.com/velrest/render_in_place_reproduction
https://ember-power-select.com/docs/api-reference

In case page is updated this was the content: