Skip to content

Update project to Angular v13#514

Open
yharaskrik wants to merge 5 commits intorintoj:masterfrom
yharaskrik:jaybell/ng-v13
Open

Update project to Angular v13#514
yharaskrik wants to merge 5 commits intorintoj:masterfrom
yharaskrik:jaybell/ng-v13

Conversation

@yharaskrik
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@SParente SParente left a comment

Choose a reason for hiding this comment

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

looks really good, and important. i would clean the any, just to be more "pretty" and more prepared for the future. added a sugestion to improve that.

Comment on lines 635 to 641
return {
top: result.top + marginTop,
bottom: result.bottom + marginBottom,
left: result.left + marginLeft,
right: result.right + marginRight,
width: result.width + marginLeft + marginRight,
height: result.height + marginTop + marginBottom
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i would avoid to use anys. i like to respect the type, just to prevent possible errors on the future. for example, if we need the x and y, and we dont use, we can pass as zero. the the toJSON can be done with a simple json stringify. see an exemple in the sugestion

Suggested change
return {
top: result.top + marginTop,
bottom: result.bottom + marginBottom,
left: result.left + marginLeft,
right: result.right + marginRight,
width: result.width + marginLeft + marginRight,
height: result.height + marginTop + marginBottom
const result = {
x: 0,
y: 0,
top: result.top + marginTop,
bottom: result.bottom + marginBottom,
left: result.left + marginLeft,
right: result.right + marginRight,
width: result.width + marginLeft + marginRight,
height: result.height + marginTop + marginBottom,
};
return {
...base,
toJSON: () => {
return JSON.stringify(base);
}
}

Copy link
Copy Markdown

@c-goldschmidt c-goldschmidt Jan 10, 2022

Choose a reason for hiding this comment

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

Since this is blocking everyone updating to angular 13 while keeping the scroller, would it be possible to go ahead with the current implementation and creating an issue out of this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @rintoj is it possible to merge this branch and release a new version?

@tiagodev
Copy link
Copy Markdown

Last year I asked for a new release, because the last one was the year before. Nothing changed so it's been 2 years now.

Is there any point for people to keep investing their time developing in this project? Unless someone forks this and starts releasing then we should all move on to another component.

@fhljys
Copy link
Copy Markdown

fhljys commented Mar 1, 2022

@iharbeck published an angular 13 version on npm:
https://www.npmjs.com/package/@iharbeck/ngx-virtual-scroller
https://github.com/iharbeck/ngx-virtual-scroller

Do we consider this one?

@fxck
Copy link
Copy Markdown

fxck commented Apr 29, 2022

any news on this?

@yharaskrik
Copy link
Copy Markdown
Author

Sorry everyone I forgot about this branch. Unfortunately I do not have permissions to merge this but I may release it under our own orgs scope and update it to angular 14, I just do not know if we want to maintain the library or not.

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.

7 participants