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

Updates to Angular 11 #146

Merged
merged 9 commits into from
Jun 8, 2021
Merged

Conversation

filipelautert
Copy link
Collaborator

@Coffee-Tea can you help to review the pull request?
Basically I updated all the dependencies to Angular 11 and fixed what was required to build and work.

Copy link

@manuelkroiss manuelkroiss left a comment

Choose a reason for hiding this comment

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

Thanks for investing time on this issue! The update to Angular 11 is highly appreciated.

I think we should maintain the original package name since otherwise it will be difficult to update.
Furthermore, the repository url should stay the same I think.

And do you thought of upgrading to Angular 12 since it is the current version?

@filipelautert
Copy link
Collaborator Author

Hi @manuelkroiss , thanks for the review. I think i didn't change the package name - in fact i changed for a while in my fork but rolled back in commit 872fed2 .
I was thinking about releasing version 2.3.x supporting Angular 11 and then create a 2.4.x for angular 12, what do you think?

@anders8
Copy link

anders8 commented Jun 7, 2021

Makes sense to have a "yep, for Angular 11, use x, for 12, use y".

anders8
anders8 previously approved these changes Jun 7, 2021
Copy link

@anders8 anders8 left a comment

Choose a reason for hiding this comment

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

Looks correct. I suspect the mousetrap include will fix many warning issues for users who haven't disabled that already.

@filipelautert
Copy link
Collaborator Author

For now the Readme cites Angular 11. After this merge we can tag 2.3.0 and use master for 2.4.x , updating the readme with those comments. If a new 2.3.x release is required we can branch from the tag.

@manuelkroiss
Copy link

Yes, providing two different versions make sense to me.

manuelkroiss
manuelkroiss previously approved these changes Jun 8, 2021
package.json Outdated
"license": "MIT",
"repository": {
"type": "git",
"url": "[email protected]:filipelautert/angular2-hotkeys.git"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it supposed to be moved to new repo?

Copy link
Collaborator Author

@filipelautert filipelautert Jun 8, 2021

Choose a reason for hiding this comment

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

no!!! my mistake, fixed.

Coffee-Tea
Coffee-Tea previously approved these changes Jun 8, 2021
Copy link
Collaborator

@Coffee-Tea Coffee-Tea left a comment

Choose a reason for hiding this comment

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

thanks @filipelautert !

only one note, I'm not sure you should update the url, see comment in PR.

@filipelautert filipelautert dismissed stale reviews from Coffee-Tea, manuelkroiss, and anders8 via 5e76586 June 8, 2021 12:14
@filipelautert filipelautert requested a review from Coffee-Tea June 8, 2021 12:15
@filipelautert filipelautert merged commit d946ba9 into brtnshrdr:master Jun 8, 2021
@pcnate
Copy link

pcnate commented Jun 9, 2021

Is the path to an npm release clear or is there other work that will be performed before this happens?

@Coffee-Tea
Copy link
Collaborator

@pcnate released to npm today. v.2.3.1, angular2-hotkeys

@pcnate
Copy link

pcnate commented Jun 9, 2021

oh, I guess I was looking for a tagged commit to appear on GitHub and for it to also show under the GitHub releases. On closer check it appears the tag happened on a branch so that's how I missed it

@manuelkroiss
Copy link

I have already updated my project to the newest version and can report that everything works fine and I could not find any bugs until now.
Thanks for your work!

@pcnate
Copy link

pcnate commented Jun 10, 2021

I can confirm it worked for me on one of two projects that uses this package.

I think what confused me is the version number was not incremented with npm version (major|minor|patch) but it appears to have been manually. @wittlock did the same on the last version but prior to that it was a tagged commit. I guess it doesn't matter as much but is more up to the maintainers preferences or build chain.

@Coffee-Tea
Copy link
Collaborator

sorry, I am actually not too experienced in npm publishes yet, and there was a tag also https://github.com/brtnshrdr/angular2-hotkeys/tags. But maybe I did smth wrong during publishing. But some notes were added to 2.3.1 tag today by @filipelautert

Those fixes (mousetrap and Angular 11) were really needed for a lot of people so we were happy to get the collaborator's rights here from the owner to get this package alive again 🎉

I think we will sort it out step by step to get more clear versioning and requested fixes for future versions with all contributors. Sorry for some inconvenions :) At least, it's unblocked for newer Angular versions now.

Thanks for the feedback @pcnate 🙏

@filipelautert
Copy link
Collaborator Author

@Coffee-Tea I saw that you released the npm package so I created the tag and release on github to keep everything in sync 👍

@anders8
Copy link

anders8 commented Jun 18, 2021

FYI, I can confirm that it's at least functional on Angular 12. I haven't dived all the way down to see if there's anything needed for the new default strict settings.

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.

5 participants