-
Notifications
You must be signed in to change notification settings - Fork 17
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
chore: update the project structure to support angular 16 #60
Conversation
changed the files and structure of the project to be more similar to current angular standards, this will enable ivy engine and thus it will work on angular 16 BREAKING CHANGE: Requires angular v16 and rxjs v7 fix #59
I found a way to compile the locales to the dist directory using tsc. Will send a PR for this. |
@frankadrian I don't think we can merge without the |
@ihym I created a proposal to use tsc to compile language-strings here: https://github.com/cyrillbrito/ngx-timeago/pull/1 |
After merging the @frankadrian PR everything seems to be working alright. |
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.
Thank you guys for the insane effort. Left some comments, I think we are close.
I'll have to migrate the CI process to gh actions in the meantime.
package.json
Outdated
"@angular/platform-browser-dynamic": "^16.0.0", | ||
"@angular/router": "^16.0.0", | ||
"bootstrap": "4.5.2", | ||
"ngx-timeago": "file:dist/ngx-timeago", |
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 guess this needs to be removed? Why not use ts paths as before?
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 tried using the paths but it was not working as expected since the tsconfig path is not the same as installing a package. The imports on the demo would look different than what people installing the package would have.
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 if we add the paths to the demo tsconfig it should work, will prepare a PR for this as well.
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.
Merged the @frankadrian PR, everything looks good. @ihym pls have a look
"@angular/compiler": "^16.0.0", | ||
"@angular/core": "^16.0.0", | ||
"@angular/forms": "^16.0.0", | ||
"@angular/material": "^16.0.1", |
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.
The demo seems a bit broken. Some small adjustments should do the trick.
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 seems to be working for me. Is it having problems on the build or something in runtime? If on the build make sure you build the lib first with the npm run build:lib
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.
Some stylings seem off.
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 some of the bootstrap styles are colliding with the material styles
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.
Hey @ihym, here is a PR to align the styling of the demo, please let us know your thoughts.
https://github.com/cyrillbrito/ngx-timeago/pull/2
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.
Merged the @frankadrian PR, everything looks good. @ihym pls have a look
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.
Eagerly awaiting to get this merged @ihym :)
I did a somewhat unorthodox release to move on. The ci setup is heavily outdated. |
Works fine together with Angular 16 afaiks. Thanks. |
Changed the files and structure of the project to be more similar to current angular standards, this will enable ivy engine and thus it will work on angular 16.
tsconfig
angular.json
BREAKING CHANGE: Requires angular v16 and rxjs v7
fix #59