-
Notifications
You must be signed in to change notification settings - Fork 9
Add Cookie Consent attribute to LFX Footer #457
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
Conversation
- Upgraded Angular from 11.2.14 to 13.4.0 - Updated @linuxfoundation/lfx-ui-core from 0.0.12 to 0.0.19 - Updated TypeScript from 4.1.6 to 4.6.4 - Updated @auth0/auth0-angular to 2.2.3 for Angular 13 compatibility - Updated ng-bootstrap from 6.2.0 to 12.1.2 - Added @popperjs/core dependency - Configured .npmrc to resolve FontAwesome registry issues - Added allowedCommonJsDependencies configuration - Build now succeeds with optional chaining syntax (?.operator) - Latest LFX UI Core features are now available - Clean builds with no parsing errors Signed-off-by: ahmedomosanya <[email protected]>
Signed-off-by: ahmedomosanya <[email protected]>
WalkthroughThis update upgrades Node.js engine requirements, Angular framework and related dependencies, and updates build scripts and configurations to align with Angular 13. It also modifies GitHub Actions workflows to use Node.js 18, adjusts polyfill imports, and adds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppComponent
participant LFXFooter
User->>AppComponent: Loads application
AppComponent->>LFXFooter: Renders <lfx-footer cookie-tracking>
LFXFooter-->>AppComponent: Footer rendered with cookie tracking enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 6
🧹 Nitpick comments (1)
angular.json (1)
29-34: Move build-time flags out of the default options block
vendorChunk,sourceMap,optimization,buildOptimizer, etc. now apply to all builds (including production) because they’re in the rootoptionsobject.
That disables optimisation for production unless explicitly overridden and slows dev rebuilds by generating extra chunks/maps.Consider:
- Keep minimal defaults in
options- Put the current flags into a dedicated
developmentconfiguration and set it as thedefaultConfiguration.This matches Angular-CLI best practices and avoids surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.gitignore(1 hunks).npmrc(1 hunks)angular.json(2 hunks)package.json(5 hunks)src/app/app.component.html(1 hunks)src/environments/environment.ts(1 hunks)src/polyfills.ts(1 hunks)src/test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Yarn Dependency Audit
package.json
[error] 1-1: Yarn install failed due to incompatible Node.js version. Module '@aws-sdk/[email protected]' requires Node >=18.0.0 but found 16.20.2.
🪛 GitHub Actions: Build
package.json
[error] 1-1: Yarn install failed due to incompatible Node.js version. Module '@aws-sdk/[email protected]' requires Node.js >=18.0.0 but found 16.20.2.
🪛 GitHub Actions: Snyk Scan Edge NPM Dependencies
package.json
[error] 1-1: Yarn install failed due to incompatible Node.js version. Module '@aws-sdk/[email protected]' requires Node >=18.0.0 but found Node 16.20.2.
🔇 Additional comments (4)
src/test.ts (1)
7-7: Zone.js Testing Import Verified – No Residual Old Paths DetectedI scanned the repository for any remaining
zone.js/dist/zone-testingimports and found none. The use ofimport 'zone.js/testing';is correct for Zone ≥ 0.11.4 (Angular 13). All good to merge.
src/polyfills.ts (1)
62-62: No remaining legacy Zone.js imports found
All occurrences of the oldzone.js/dist/zoneimport have been removed—only the rootimport 'zone.js';remains. 🎉src/environments/environment.ts (1)
26-26: Comment updated to new plugin path – looks goodThe comment now points to
zone.js/plugins/zone-error, matching the new package layout..gitignore (1)
76-76: .angular/ added to .gitignore – good callExcluding the Angular cache directory keeps commits clean and CI lean.
…patibility Signed-off-by: ahmedomosanya <[email protected]>
87bf67c to
a9de464
Compare
… compatibility Signed-off-by: ahmedomosanya <[email protected]>
Signed-off-by: ahmedomosanya <[email protected]>
- Add resolutions for @cypress/request, form-data, braces, micromatch, cross-spawn, brace-expansion - Update axios, webpack, esbuild, @babel/runtime, webpack-dev-server to secure versions - Reduce vulnerabilities from 11 critical/high/moderate to 1 moderate (Bootstrap XSS) - yarn_audit.sh now passes (only fails on critical vulnerabilities >=16) The remaining Bootstrap vulnerability requires upgrading to v5 which would be a breaking change for UI components and should be handled in a separate PR. Signed-off-by: ahmedomosanya <[email protected]>
- Add security resolutions to test/functional/package.json - Fix @cypress/request, form-data, braces, micromatch, cross-spawn, brace-expansion Signed-off-by: ahmedomosanya <[email protected]>
- Remove duplicate configuration flags from build:dev, build:staging, build:prod scripts - Original scripts used --prod --configuration=dev (duplicate configs since --prod = --configuration=production) - ng update converted --prod to --configuration production, making the duplication explicit - Angular CLI was already using last config and showing warning about override - Each environment config in angular.json already includes production optimizations Before: ng build --configuration production --configuration=dev After: ng build --configuration dev This is the first time these build scripts are semantically correct Signed-off-by: ahmedomosanya <[email protected]>
Signed-off-by: ahmedomosanya <[email protected]>
- Remove 'defaultConfiguration: ""' which causes Angular CLI to fail - Empty string is invalid, Angular CLI expects non-empty configuration name - Base build options already have development-friendly defaults (sourceMap: true, optimization: false) - Angular CLI will use base configuration when no specific configuration is specified Fixes error: 'Configuration "" could not be found for project' Signed-off-by: ahmedomosanya <[email protected]>
amolsontakke3576
left a comment
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.
LGTM just make sure its run properly.
Summary by CodeRabbit
New Features
Chores