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

Update project to Angular v13 #514

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yharaskrik
Copy link

No description provided.

Copy link

@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

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

@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?

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

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

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

fxck commented Apr 29, 2022

any news on this?

@yharaskrik
Copy link
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