Split state and record for a list item, fixed #129 #130
Conversation
|
I think it's a major since I change a few interface for the components. Let me know if you are not agreeing. Also feel free to propose different names for the interface attributes. |
|
Changes Unknown when pulling 9462e67 on dafortin:master into ** on ciena-frost:master**. |
|
We need to do everything possible avoid #major# releases - can you summarize the breaking changes? |
|
I think we can probably do a #patch# since the interfaces changes are for components that I'm not expecting people to use on their own. |
|
Changes Unknown when pulling 9e6e12b on dafortin:master into ** on ciena-frost:master**. |
|
Changes Unknown when pulling 37c3b45 on dafortin:master into ** on ciena-frost:master**. |
addon/components/frost-list.js
Outdated
| @@ -129,17 +129,6 @@ export default Component.extend({ | |||
|
|
|||
| // == Lifecycle Hooks ======================================================= | |||
|
|
|||
| didUpdateAttrs ({newAttrs}) { | |||
There was a problem hiding this comment.
Sorry - can you put a quick explanation on the removal of this?
There was a problem hiding this comment.
We don't need the frost scroll with the vertical-collection. The vertical collection has the scrolling capability.
There was a problem hiding this comment.
Did you push the changeset? It still appears as removed in the PR
|
|
||
| {{#if pagination}} |
There was a problem hiding this comment.
Is it necessary to remove the "basic" {{#each}} loop in the pagination case? I'd prefer to not introduce {{#vertical-collection}} for those people using the pagination scenario in a minor release
There was a problem hiding this comment.
I can remove it. I thought you asked me to replace the each by a vertical-collection.
There was a problem hiding this comment.
Yeah, I'd like to keep the existing pagination so that we don't risk breaking
There was a problem hiding this comment.
Ping me when that's done and I'll merge
There was a problem hiding this comment.
Sure I reverted this :)
There was a problem hiding this comment.
Same here, still appears as removed in the PR
| containerSelector=null | ||
| containerHeight=null | ||
| key='@identity' | ||
| resizeDebounce=64 |
There was a problem hiding this comment.
Not sure what this is. I simply used the same vertical collection as below... I feel like we have a lot of properties that might not be necessary in the vertical collection.
There was a problem hiding this comment.
Ok, that's fine - it's like 15 FPSish - just an odd default, but no problem
|
Looks good overall, just a few comments |
|
Changes Unknown when pulling e008455 on dafortin:master into ** on ciena-frost:master**. |
dafortin
changed the title
|
@sglanzer pushed the changes |
|
Changes Unknown when pulling f403e20 on dafortin:master into ** on ciena-frost:master**. |
|
Approved! |
This project uses semver, please check the scope of this pr:
CHANGELOG
smoke-and-mirrors