License Page New UI #1535
License Page New UI #1535
Conversation
WalkthroughThis pull request restructures the license detail component. The Handlebars template has been refactored from a table layout to a stack-based design using components such as Changes
Possibly related PRs
Suggested Reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/components/license-detail/index.ts (2)
17-23: Consider adding date format validation.While the optional chaining is good for null safety, consider adding validation for the date format to handle potential invalid dates.
get startDate() { + const date = this.args.license?.startDate; + if (!date || !dayjs(date).isValid()) { + return this.intl.t('notAvailable'); + } return dayjs(this.args.license?.startDate).format('DD MMM YYYY'); } get expiryDate() { + const date = this.args.license?.expiryDate; + if (!date || !dayjs(date).isValid()) { + return this.intl.t('notAvailable'); + } return dayjs(this.args.license?.expiryDate).format('DD MMM YYYY'); }
25-49: Consider extracting feature details into a separate method.The
feturesDetailsgetter (note the typo in the name) contains complex conditional logic. Consider extracting it into a separate method for better maintainability.+ private getFeatureDetails(isLimitedScans: boolean) { + return isLimitedScans + ? [ + { + label: this.intl.t('noOfScans'), + value: this.args.license?.perScanQuantity, + }, + { + label: this.intl.t('planType'), + value: this.args.license?.perScanDescription, + }, + ] + : [ + { + label: this.intl.t('noOfApps'), + value: this.args.license?.perAppQuantity, + }, + { + label: this.intl.t('planType'), + value: this.args.license?.perAppDescription, + }, + ]; + } - get feturesDetails() { + get featureDetails() { - if (this.args.license?.isLimitedScans) { - return [/*...*/]; - } - return [/*...*/]; + return this.getFeatureDetails(this.args.license?.isLimitedScans ?? false); }translations/en.json (1)
940-940: New Localization Key: "noOfScans"The
"noOfScans": "No of Scans"entry succinctly communicates a scan count. Verify with the UI/UX team if the phrasing is final—sometimes, "Number of Scans" or "No. of Scans" might be preferred for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/license-detail/index.hbs(1 hunks)app/components/license-detail/index.scss(1 hunks)app/components/license-detail/index.ts(2 hunks)app/styles/_component-variables.scss(1 hunks)app/templates/authenticated/dashboard/billing.hbs(1 hunks)translations/en.json(3 hunks)translations/ja.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/templates/authenticated/dashboard/billing.hbs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
app/components/license-detail/index.hbs (2)
1-20: LGTM! The header section is well structured.The header layout using
AkStackwith justified content and aligned items provides a clean and responsive design. The conditional rendering of license type is handled appropriately.
22-42: LGTM! The content section uses a consistent and maintainable pattern.The use of
{{#each}}withAkStackcomponents creates a scalable and maintainable structure for displaying license sections. The consistent spacing and typography enhance readability.app/components/license-detail/index.ts (2)
1-16: LGTM! Proper type imports and service injection.The code correctly imports types and injects the intl service for internationalization.
51-81: LGTM! Well-structured license sections getter.The
licenseSectionsgetter provides a clean and maintainable way to organize the license information into sections.app/components/license-detail/index.scss (1)
1-15: LGTM! Clean and maintainable styles.The styles are well-organized and use CSS variables for colors and consistent spacing. The use of
emunits for padding and margin ensures proper scaling.translations/en.json (2)
471-471: New Localization Key: "feature"The addition of the
"feature": "Feature"key is clear and straightforward. It aligns with the established style for translation values in the file. Please ensure that any UI component referencing this key (likely related to the new license page UI) provides adequate context for translators if needed.
1144-1144: New Localization Key: "planType"The
"planType": "Plan Type"key is a valuable addition to support the new License Page UI elements. Ensure that all components referencing this key consistently follow the same naming conventions across both localization files (e.g., English and Japanese).app/styles/_component-variables.scss (1)
1999-2002: Update to License Detail Background ColorA new custom property
--license-detail-background-coloris introduced and set tovar(--common-white). This change replaces the previously defined--license-detail-success-lightand--license-detail-primary-lightvariables. Please verify that:
- The updated variable is consistently used throughout the license detail component as per the new UI design.
- All references to the now-removed variables have been updated across the codebase to avoid styling inconsistencies.
Deploying irenestaging with
|
| Latest commit: |
5eaef1b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1ea7dc27.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1707-update-license-page.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1707-Update-License-Page-UI
|
| Run status |
|
| Run duration | 07m 34s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
2
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
1
|
Passing
|
29
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
upload-app.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Upload App > It successfully uploads an apk file |
Test Replay
Screenshots
|
|
dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 58062 |
Test Replay
Screenshots
|
|
SmitGala
force-pushed
the
PD-1707-Update-License-Page-UI
branch
from
0f838da to
c81cbe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/license-detail/index.ts (1)
30-53: Consider adding null checks for optional properties.While optional chaining is used for accessing properties, the array is still returned even if
this.licenseis null. Consider adding a null check to return an empty array in such cases.get featuresDetails() { + if (!this.license) { + return []; + } + if (this.license?.isLimitedScans) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/license-detail/index.hbs(1 hunks)app/components/license-detail/index.scss(1 hunks)app/components/license-detail/index.ts(2 hunks)app/styles/_component-variables.scss(1 hunks)app/templates/authenticated/dashboard/billing.hbs(1 hunks)translations/en.json(3 hunks)translations/ja.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/templates/authenticated/dashboard/billing.hbs
- app/components/license-detail/index.scss
- translations/ja.json
- app/styles/_component-variables.scss
- translations/en.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
app/components/license-detail/index.hbs (2)
1-20: LGTM! Clean and modern header implementation.The header section effectively uses modern components and provides clear visual hierarchy with proper spacing.
22-42: LGTM! Well-structured and maintainable body implementation.The body section effectively uses iteration and modern components to create a clean, maintainable layout. The use of AkDivider provides clear visual separation between sections.
app/components/license-detail/index.ts (2)
1-7: LGTM! Clean imports following previous feedback.The imports are well-organized and follow the previous review feedback about using direct service import.
17-19: LGTM! License getter added as requested.The license getter has been added as suggested in the previous review.
SmitGala
force-pushed
the
PD-1707-Update-License-Page-UI
branch
from
c81cbe2 to
5eaef1b
Compare
|
No description provided.