Conversation
Update an Angular CLI project to version 8.
Angular Workspace migration. Update an Angular CLI workspace to version 9.
Replace deprecated 'styleext' and 'spec' Angular schematic options.
…ated SASS imports
…ease-5.1.0_v9 Issue #SB-0000 fix: Upgrade library from Angular 8 to Angular 9
Update tslint to version 6 and adjust rules to maintain existing behavior.
Update library projects to use tslib version 2 as a direct dependency. Read more about this here: https://v10.angular.io/guide/migration-update-libraries-tslib
Update workspace dependencies to match a new v10 project.
Update 'module' and 'target' TypeScript compiler options. Read more about this here: https://v10.angular.io/guide/migration-update-module-and-target-compiler-options
Issue #SB-0000 fix: Upgrade library from Angular 9 to Angular 10
Issue #SB-0000 fix: Upgrade library from Angular 9 to Angular 10
Replace deprecated library builder '@angular-devkit/build-ng-packagr'.
Add 'declarationMap' compiler options for non production library builds.
Update workspace dependencies to match a new v11 project.
Issue #SB-0000 fix: Upgrade library from Angular 10 to Angular 11
Update 'zone.js' to version 0.11.x. Read more about this here: https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md#breaking-changes-since-zonejs-v0111
Remove 'emitDecoratorMetadata' TypeScript compiler option. Decorator metadata is no longer needed by Angular. Read more about this here: https://www.typescriptlang.org/docs/handbook/decorators.html#metadata
Optional migration to update Angular CLI workspace configurations to 'production' mode by default.
…tion Issue #ED-3735 Angular migration from 15 to 16
…tion Angular migration 16 without legacy peers
SBCOSS-I251: Display description for notifications
V16 changes
SBCOSS-I299: Changes to suporrt angular material v14
WalkthroughMajor version upgrade from Angular 8 to Angular 16, including dependency updates, component refactoring to integrate Material Design, configuration updates for the new build system, CI/CD pipeline migration from CircleCI to GitHub Actions, and TypeScript compiler option changes for Ivy and partial compilation mode. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/notification/src/lib/notification-card/notification-card.component.html (1)
14-34: Remove commented-out code before merging.The old implementation should be removed. If reference is needed, it remains available in git history.
🔎 Proposed fix
</mat-card> - -<!-- <div class="sb-notification-list p-8" (click)="notificationClickHandler($event)" tabindex="0"> - <div class="sb-notification-list-header d-flex flex-ai-center flex-jc-space-between"> - <div class="notification-info d-flex flex-ai-center mb-8"> - <div class="status" *ngIf="(notification?.status === NotificationStatus.UNREAD)"></div> - <div class="date ml-8">{{notification?.createdOn | date:'E, d MMMM h:mm'}}</div> - </div> - <div class="delete-icon" *ngIf="!hideDeleteOption" (click)="notificationDeleteHandler($event);$event.stopPropagation()" tabindex="0"> - <img class="Delete-gray" src="assets/common-consumption/images/Delete-gray.svg" alt="" alt="delete" width="14px"> - <img class="Delete-red" src="assets/common-consumption/images/Delete-red.svg" alt="" alt="delete" width="14px"> - </div> - </div> - <div class="sb-notification-data d-flex flex-ai-center"> - <div class="logo" *ngIf="notification?.data?.actionData?.thumbnail"> - <img [src]="notification?.data?.actionData?.thumbnail" width="20px"> - </div> - <div class="ml-12 notification-content"> - <div class="title" *ngIf="notification?.action?.template?.data?.title" [ngClass]="{'active': notification?.status === NotificationStatus.UNREAD}">{{notification?.action?.template?.data?.title}}</div> - <div class="sub-title py-8" *ngIf="notification?.data?.actionData?.description">{{notification?.data?.actionData?.description}}</div> - </div> - </div> -</div> -->
🧹 Nitpick comments (13)
projects/notification/src/lib/notification.component.html (1)
17-18: Remove redundant accessibility attributes.Both show more/less buttons have redundant attributes:
aria-hidden="false"- elements are visible by defaulttabindex="0"- Material buttons are already in the tab order🔎 Proposed cleanup
- <a mat-button color="accent" aria-hidden="false" class="text-center" *ngIf="displayItemCount < this.notificationList?.length" tabindex="0" (click)="onShowMore()">{{inAppNotificationConfig?.moreText}}</a> - <a mat-button color="accent" aria-hidden="false" class="text-center" *ngIf="displayItemCount === this.notificationList?.length" tabindex="0" (click)="onShowLess()">{{inAppNotificationConfig?.lessText}}</a> + <a mat-button color="accent" class="text-center" *ngIf="displayItemCount < this.notificationList?.length" (click)="onShowMore()">{{inAppNotificationConfig?.moreText}}</a> + <a mat-button color="accent" class="text-center" *ngIf="displayItemCount === this.notificationList?.length" (click)="onShowLess()">{{inAppNotificationConfig?.lessText}}</a>projects/notification/src/test.ts (1)
16-18: Consider enablingdestroyAfterEach: truefor better test isolation.The current configuration
destroyAfterEach: falsepreserves Angular 8's behavior where test modules are not destroyed between tests. While this helps during migration, Angular 16 recommendsdestroyAfterEach: truefor better test isolation and to prevent state leakage between tests.🔎 Recommended configuration for Angular 16
getTestBed().initTestEnvironment( BrowserDynamicTestingModule, platformBrowserDynamicTesting(), { - teardown: { destroyAfterEach: false } + teardown: { destroyAfterEach: true } } );projects/notification/src/lib/notification-card/notification-card.component.scss (1)
1-1: Consider removing the commented import.The old
@importstatement is commented out but can be safely removed now that the migration to@useis complete.🔎 Proposed cleanup
-// @import "../../../../../node_modules/@project-sunbird/sb-styles/assets/mixins/mixins"; @use "@project-sunbird/sb-styles/assets/mixins/mixins" as *;projects/notification/tsconfig.lib.prod.json (1)
6-9: Remove redundant angularCompilerOptions.The
compilationModeandenableIvysettings are already inherited fromtsconfig.lib.json, making this block unnecessary.🔎 Proposed simplification
{ "extends": "./tsconfig.lib.json", "compilerOptions": { "declarationMap": false - }, - "angularCompilerOptions": { - "compilationMode": "partial", - "enableIvy": true } }projects/notification/README.md (2)
40-42: Add language specifier to code block.The installation code block should specify
bashorshellfor proper syntax highlighting.🔎 Proposed fix
-``` +```bash $ npm i @project-sunbird/sb-notification</details> --- `89-101`: **Replace hard tabs with spaces in version table.** The version table uses hard tabs which violates markdown best practices. Replace with spaces for consistent rendering across editors. <details> <summary>🔎 Proposed fix</summary> ```diff -| release branch | npm package version | Angular Version | -|:-----------------: |:-------------------: |:---------------: | -| release-5.1.0_v9 | 5.0.2 | NG V9 | +| release branch | npm package version | Angular Version | +|:-----------------:|:-------------------:|:---------------:| +| release-5.1.0_v9 | 5.0.2 | NG V9 |Apply similar changes to all remaining rows in the table.
projects/notification/package.json (1)
16-18: Consider adding peerDependencies for Angular compatibility.Removing
peerDependenciesfor Angular libraries is atypical. Libraries should declare peer dependencies on@angular/coreand@angular/commonto ensure consumers use compatible Angular versions and receive warnings when there's a mismatch.🔎 Suggested addition
"dependencies": { "tslib": "2.6.2" }, + "peerDependencies": { + "@angular/common": "^16.0.0", + "@angular/core": "^16.0.0" + }, "homepage": "https://github.com/Sunbird-Ed/sb-notification.git#readme"README.md (3)
40-42: Add language specifier to code block.The installation code block should specify a language for proper syntax highlighting.
🔎 Proposed fix
-``` +```bash $ npm i @project-sunbird/sb-notification</details> --- `46-65`: **Use correct language identifier for TypeScript code.** The code block uses `console` as the language identifier, but the content is TypeScript. This affects syntax highlighting. <details> <summary>🔎 Proposed fix</summary> ```diff -```console +```typescript import { BrowserModule } from '@angular/platform-browser';
89-101: Update versions table and fix formatting.
- The table uses hard tabs instead of spaces, which can cause rendering issues in some Markdown viewers.
- The current release
8.1.0(per PR title) is missing from the versions table.Consider adding the new version row and replacing tabs with spaces for consistency.
package.json (3)
37-37: Consider migrating from deprecated linting tools.
codelyzerandtslintare deprecated since Angular 12. The Angular team recommends migrating to@angular-eslint. This isn't blocking for this release, but should be addressed in a future update.Also applies to: 48-48
46-46: Protractor is deprecated.Protractor is no longer maintained and Angular has removed it from new projects. Consider migrating to Cypress, Playwright, or WebdriverIO for e2e testing in a future update.
25-25: Consider updating core-js.
core-jsversion2.5.4is from 2018. If polyfills are still needed for your target browsers, consider updating tocore-js@3.x. However, with Angular 16 targeting modern browsers, you may be able to remove this dependency entirely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.circleci/config.yml.github/workflows/publish-npm.yaml.gitignoreREADME.mdangular.jsonpackage.jsonprojects/notification/README.mdprojects/notification/package.jsonprojects/notification/src/lib/notification-card/notification-card.component.htmlprojects/notification/src/lib/notification-card/notification-card.component.scssprojects/notification/src/lib/notification.component.htmlprojects/notification/src/lib/notification.component.scssprojects/notification/src/lib/notification.module.tsprojects/notification/src/public-api.tsprojects/notification/src/test.tsprojects/notification/tsconfig.lib.jsonprojects/notification/tsconfig.lib.prod.jsontsconfig.jsontslint.json
💤 Files with no reviewable changes (1)
- .circleci/config.yml
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
projects/notification/README.md
5-5: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Dollar signs used before commands without showing output
(MD014, commands-show-output)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
89-89: Hard tabs
Column: 21
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 44
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 63
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 21
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 44
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 63
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 21
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 44
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 63
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 22
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 45
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 64
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 22
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 45
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 64
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 22
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 45
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 64
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 22
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 45
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 64
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 22
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 45
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 64
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 22
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 45
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 64
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 22
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 45
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 63
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 22
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 45
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 63
(MD010, no-hard-tabs)
README.md
5-5: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Dollar signs used before commands without showing output
(MD014, commands-show-output)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
89-89: Hard tabs
Column: 21
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 44
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 63
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 21
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 44
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 63
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 21
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 44
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 63
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 22
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 45
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 64
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 22
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 45
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 64
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 22
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 45
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 64
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 22
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 45
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 64
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 22
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 45
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 64
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 22
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 45
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 64
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 22
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 45
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 63
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 22
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 45
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 63
(MD010, no-hard-tabs)
🔇 Additional comments (19)
projects/notification/src/lib/notification.component.html (1)
9-14: LGTM!The notification list structure correctly uses Angular's
*ngForwith theslicepipe for pagination, properly binds data to thesb-notification-cardcomponent, and includes appropriate event handlers. Thetabindex="0"ensures keyboard accessibility for each notification card.projects/notification/src/test.ts (2)
23-23: Summary inconsistency detected.The AI-generated summary states that this line was "added," but the code annotations show it was already present (no
~marker). This line appears to have existed in the previous version of the file.
3-4: Zone.js import paths are correct for Angular 16.The import statements have been properly updated to remove the
/dist/prefix, aligning with the modern zone.js package structure. However, verify that the zone.js version in package.json is 0.14.0 or later (not 0.13.0), as Angular v16 supports Zone.js > version 0.14.x or later.projects/notification/src/lib/notification-card/notification-card.component.scss (1)
2-7: Modern Sass migration looks good — package dependency verified.The migration from
@importto@usealigns with modern Sass best practices (introduced in 2019) and reduces compiled CSS output. The@project-sunbird/sb-stylesv0.0.16 dependency is properly configured in package.json. The simplified styling approach leverages Material Design defaults effectively.Note: The repository does not currently have visual regression test infrastructure configured. Manual testing or verification through existing component tests is recommended to ensure the simplified styling maintains expected UI appearance across different themes and screen sizes.
.gitignore (1)
35-35: LGTM! Standard Angular 16+ cache directory.This addition appropriately excludes the Angular CLI build cache from version control.
projects/notification/tsconfig.lib.json (2)
5-5: LGTM! Enables source map navigation for published library.The
declarationMap: trueoption helps consumers navigate to the original TypeScript source when debugging.
26-27: LGTM! Appropriate Angular 16+ library configuration.Enabling Ivy with partial compilation mode is the standard approach for Angular 16+ libraries, allowing the library to be compatible with different Angular applications.
tsconfig.json (2)
4-5: LGTM! Correct Ivy configuration for Angular 16.Setting
enableIvy: "partial"withcompilationMode: "partial"is the standard configuration for Angular 16+ projects with publishable libraries.
9-9: LGTM! Modern JavaScript target configuration.The upgraded module (es2020), target (es2015), and downlevelIteration settings are appropriate for Angular 16+ and modern browser support.
Also applies to: 13-13, 17-17
projects/notification/src/lib/notification.module.ts (1)
5-7: LGTM! Angular Material integration.The Material module imports (Button, Card, Icon) are correctly added to support the updated UI components.
Also applies to: 16-18
tslint.json (1)
120-125: LGTM! Updated linting rules for Angular 16.The rule name updates align with the latest codelyzer conventions for Angular 16+, maintaining the same enforcement semantics with modernized rule names.
.github/workflows/publish-npm.yaml (1)
1-39: Workflow configuration verified and correct.The
ng buildcommand via ng-packagr outputs todist/notificationwith a complete npm package (including package.json) ready for publishing. Thenpm packandnpm publishcommands targeting./dist/notificationare appropriately configured. Ensure theNPM_TOKENsecret is configured in your repository settings with appropriate npm registry permissions.projects/notification/package.json (1)
3-3: Version mismatch with PR title.The package version is
8.0.1-v16.1but the PR title indicates "Release 8.1.0". Please verify the intended version.angular.json (2)
42-44: Verify the analytics setting is intentional.The CLI analytics is enabled with a specific UUID. Ensure this is intentional for the project. If you prefer to disable telemetry, you can set
"analytics": false.
12-25: LGTM!The builder migration from
@angular-devkit/build-ng-packagr:buildto@angular-devkit/build-angular:ng-packagris the correct approach for Angular 16+. The production/development configurations with separate tsconfig files follow Angular best practices.projects/notification/src/public-api.ts (1)
8-8: LGTM!The new export correctly expands the public API to include the
notification-cardcomponent, following the existing export pattern. The component is properly implemented and ready for use.projects/notification/src/lib/notification-card/notification-card.component.html (3)
7-11: LGTM!The actions section correctly implements event propagation control, includes proper accessibility attributes (
aria-label), and appropriately uses Angular Material components.
1-12: Remove verification request—thumbnail removal is part of intentional design refactoring.The old implementation (lines 14-34, commented out) displays notification thumbnails using
notification?.data?.actionData?.thumbnail. The new mat-card implementation intentionally removes this feature as part of a broader redesign that also shifts the data model fromnotification.data.actionDatatonotification.action.template.data. The simplified mat-card design no longer includes thumbnail/logo display. This is a deliberate design change, not a regression.
4-6: The data structure refactoring appears complete. The new pathnotification?.action?.template?.data?.descriptionis correctly implemented with JSON parsing innotification.component.ts, and the oldnotification.data.actionDatastructure is no longer used in active code.The old structure exists only in commented code and legacy test data (notification-data.ts), not in any active implementation. The JSON transformation at lines 37-38 of notification.component.ts properly converts
action.template.datafrom a JSON string to a parsed object, enabling the current template bindings to work correctly. No consumers are using the deprecated path.
projects/notification/src/lib/notification-card/notification-card.component.html
Show resolved
Hide resolved
projects/notification/src/lib/notification-card/notification-card.component.html
Show resolved
Hide resolved
8 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.