-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feature/support-angular-20 #2049
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?
Conversation
…ove ComponentFactoryResolver (breaking change)
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@swimlane/ngx-charts", | |||
| "version": "22.0.0", | |||
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 22.0.0 is already a new alpha version, if you look at the Releases section.
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 was iterating based on the changelog comments. v22.0.0 is on npm (i use it in a project w/o the alpha designation). Figured v20 should bump major version.
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.
yeah, I've been using v22 stable release and expecting v23 to support angular v20
PS: would be really great if we can sync our versions with angular version
|
|
||
| const portalHost = new DomPortalOutlet( | ||
| appendLocation, | ||
| this.componentFactoryResolver, |
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.
Would this not break in Angular v17? See comment doc for this param in v17 here
It states that this is required for attaching component portal and this is what we are doing here
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 suppose it would break on prior versions where that param is required/expected. I'm not sure how this project handles dependencies that break between Angular versions in terms of releasing on npm.
If one of the maintaners steps in, i'm happy to target a new branch for the PR or whatever is necessary to get this merged in so that it actually does work in Angular 20.
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.
@marjan-georgiev @amcdnl any suggestions on how we should proceed with the update?
My suggestion is that we drop support for Angular v17 on v23
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.
v17 is no longer supported by Angular anyway
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.
Perfect! One more reason to drop support for v17
|
@cockdiesel I believe we also need to update the changelog.md For reference, see this PR |
|
@cockdiesel It's going to be annoying, but I think we also should upgrade the demo app to v20 : ) Is there any way I can push commits to this PR? I am also willing to contribute to get this across the line CC: @seandgrimes, @marjan-georgiev, @youngcm2. Can anyone jump in here and provide a checklist to review? |
|
@seandgrimes @marjan-georgiev @youngcm2 is there something missing here? |
Can someone mention who is responsible for looking into these PRs |
|
@steveblue are there plans to update to v20 support, with this PR or otherwise? |
@rarDevelopment @yousafravian @cockdiesel |
|
@MarkColeman1 Do we have an idea when v23 will be released? I see an alpha version, but would feel safer waiting for the full release |
@zacharyjorn I can't make any promises but it will likely be late August/early September. |
What kind of change does this PR introduce? (check one with "x")
Support Angular 20; fix Angular breaking change (The constructor of
DomPortalOutlethas changed)What is the current behavior? (You can also link to an open issue here)
Tooltip broken #2046
What is the new behavior?
Tooltip works
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: