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

Vertical Collection when collection starts off screen #151

Closed

Conversation

kylemellander
Copy link
Contributor

Closes #150

This simply creates a floor for the index of the lastVisible Component Index which was causing errors when the vertical collection originates off the screen.

@eriktrom
Copy link
Member

@kylemellander are u actively using this in a large app? If so talk to @runspired - i know he's busy with a lot of things, maybe u can maintain/help this one for a bit if you understand and use it.

(just a friendly thought - runspired has awesome work all over github, more than he can maintain for sure)

@luxzeitlos
Copy link

I'm hacking this into our app with an initializer currently. It would be awesome to get this fixed.

@ssendev
Copy link
Contributor

ssendev commented Apr 28, 2017

@runspired can you merge this please?

I just debugged this in my app and wanted to send a pull request only to find out there is already one.

If you want to know what's going on: this break https://github.com/runspired/smoke-and-mirrors/blob/04a89de319948200ab996d6e126b70922716a139/addon/components/vertical-collection/component.js#L486 can be hit on the first iteration causing item to be null https://github.com/runspired/smoke-and-mirrors/blob/04a89de319948200ab996d6e126b70922716a139/addon/components/vertical-collection/component.js#L543

@runspired
Copy link
Collaborator

@ssendev actually, could you check if this is fixed by the new version of @html-next/vertical-collection? We've theorized that it is based on our understanding of the issue but would be nice to confirm

@ssendev
Copy link
Contributor

ssendev commented Apr 29, 2017

Unfortunately it dosn't work.

I removed smoke-and-mirors installed @html-next/vertical-collection@1.0.0-beta.3 and replaced

<style>
  vertical-collection {
    display: flex;
    flex-wrap: wrap;
    justify-content: center;
  }
  my-component {
    width: 24rem;
    height: 31rem;
    margin: 1rem
  }
</style>
{{#vertical-collection
    content=items
    defaultHeight='32.875rem'
    containerSelector='.mdl-layout'
    invisibleBuffer=0
    visibleBuffer=1.5
  as |item| }}
    {{component myComponent item=item}}
{{/vertical-collection}}

with

{{#vertical-collection
    items
    minHeight='32.875rem'
    containerSelector='.mdl-layout'
  as |item| }}

which resulted in You called estimateElementHeight without an element setting minHeight to 526 which is the rem value in pixels resulted in item height must always be above minimum value. Item 1 measured: 0. Setting minHeight to 0 results in targetValue must be greater than or equal to 0

Removing flex changes the error to item height must always be above minimum value. Item 0 measured: 496 which is the rem height in pixels. When setting minHeight to 496 the error disappears but items are being culled while they are still visible. Setting bufferSize=2 fixes that. But when scrolling very seldomly errors like item height must always be above minimum value. Item 28 measured: -3072 or TypeError: Cannot read property 'getBoundingClientRect' of undefined appear.

Not using the component helper and specifying a component directly dosn't seem to help either.

@runspired
Copy link
Collaborator

@ssendev thanks! I'm going to copy your response to an issue there to track it for 1.0. We should be bumping smoke-and-mirrors to use vertical-collection here in the next day or so :)

@pzuraq
Copy link

pzuraq commented Jul 25, 2017

@ssendev we've added a test for this in 1.0.0-beta.5, it should be working! Let us know if not, we may need to do a more in-depth debugging session

@runspired runspired closed this Apr 8, 2018
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

6 participants