Frontend/privilege verification enhancements #857
Conversation
|
""" WalkthroughThis update introduces QR code generation and public profile linking to the LicenseeProof page, displays full license type names, and shows privilege ID numbers. It updates styles and localization for these features, modifies the License model's display logic, and adjusts the PageContainer to handle header visibility for the LicenseeVerification route. New dependencies for QR code functionality are added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LicenseeProofComponent
participant QRCodeLibrary
participant Router
participant EnvConfig
User->>LicenseeProofComponent: Page loads
LicenseeProofComponent->>Router: Resolve public profile route
Router-->>LicenseeProofComponent: Return route path
LicenseeProofComponent->>EnvConfig: Get domain
EnvConfig-->>LicenseeProofComponent: Return domain
LicenseeProofComponent->>QRCodeLibrary: Generate QR code for public profile URL
QRCodeLibrary-->>LicenseeProofComponent: Return QR code data URL
LicenseeProofComponent-->>User: Display QR code and public profile link
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
webroot/src/components/Page/PageContainer/PageContainer.ts (1)
47-54: 💡 Verification agent❓ Verification inconclusive
Header-visibility logic appears inverted for non-phone break-points
includePageHeaderreturnstrueonly whenisPhoneistrue. That means the page header is never rendered on tablet / desktop, even when the route is not innonHeaderRouteNames. Verify this is intentional – the new requirement only calls for hiding nav on mobile, not suppressing the header everywhere.return (this.isPhone && !nonHeaderRouteNames.includes(this.currentRouteName));If desktop/tablet should still show the header, drop the
this.isPhone &&guard.
Header rendering disabled on tablet/desktop breakpoints
TheincludePageHeadergetter only returnstruewhenisPhoneistrue, so on tablet and desktop the page header never appears—even for routes not innonHeaderRouteNames. If the goal is to hide the header only on mobile and still show it elsewhere (except on logout or verification routes), drop thethis.isPhoneguard:- return this.isPhone && !nonHeaderRouteNames.includes(this.currentRouteName); + return !nonHeaderRouteNames.includes(this.currentRouteName);• File: webroot/src/components/Page/PageContainer/PageContainer.ts (lines 47–54)
🧹 Nitpick comments (8)
webroot/package.json (1)
31-32: Pin the new QR-code dependencies for reproducible buildsAll fixed-production deps in this file (e.g.
axios,vue) are version-pinned, whereas the newly-added libraries are added with the caret range.
To stay consistent with the existing lock-down strategy and avoid unexpected breakages from a future1.xofqrcode, pin both libs:- "qrcode": "^1.5.4", + "qrcode": "1.5.4", - "@types/qrcode": "^1.5.5", + "@types/qrcode": "1.5.5",After changing, run
yarn install && yarn auditto confirm no new advisories appear.Also applies to: 52-53
webroot/src/styles.common/_colors.less (1)
80-85: Consider a fallback for the exported CSS variable
--primary-coloris compiled to the literal value of@primaryColor, which means theming the color at runtime requires overriding the variable and keeping this Less value in sync.If the intent is to allow dynamic override, expose the variable first and then consume it elsewhere, e.g.
:root { --primary-color: @primaryColor; // default } /* usage */ .some-class { color: var(--primary-color, @primaryColor); }This keeps existing styling intact while letting themes override
--primary-colorwithout recompilation.webroot/src/models/License/License.model.ts (1)
144-148: Minor optimisation & readability fordisplayName
this.issueState?.name()is invoked twice and the intent of the boolean flag is not self-evident. A small refactor improves clarity and avoids the duplicate call:- public displayName(delimiter = ' - ', displayAbbrev = false): string { - const licenseTypeToShow = displayAbbrev ? this.licenseTypeAbbreviation() : this.licenseType; - - return `${this.issueState?.name() || ''}${this.issueState?.name() && licenseTypeToShow ? delimiter : ''}${licenseTypeToShow || ''}`; - } + public displayName(delimiter = ' - ', displayAbbrev = false): string { + const stateName = this.issueState?.name() || ''; + const licenseTypeToShow = displayAbbrev ? this.licenseTypeAbbreviation() : this.licenseType; + + return `${stateName}${stateName && licenseTypeToShow ? delimiter : ''}${licenseTypeToShow || ''}`; + }Also consider replacing the positional
displayAbbrevflag with an options object for future extensibility (e.g.{ delimiter: ' - ', abbrev: true }).webroot/src/components/Page/PageContainer/PageContainer.less (1)
33-40: Missing print-media override for.no-page-header
You added margin/padding tweaks for normal display but not for@media print(which is defined earlier in the same rule). Printed output for the verification page will therefore have an extramargin-top: @appHeaderHeight, shifting everything down. Add a print override similar to the one that exists for the base rule.webroot/src/components/Page/PageContainer/PageContainer.vue (1)
8-12: Class binding order nit
Because bothno-top-padandno-page-headercan manipulate top spacing, consider placing the more specificno-page-headerclass first to guarantee cascade priority when both evaluate totrue.- 'no-top-pad': !shouldPadTop, - 'no-page-header': !includePageHeader + 'no-page-header': !includePageHeader, + 'no-top-pad': !shouldPadTopwebroot/src/pages/LicenseeProof/LicenseeProof.ts (2)
95-119: HardenpublicProfileUrlconstruction and avoid fragile string concatenation
domainmay or may not include protocol / trailing slash, leading to malformed URLs.new URL()already accepts a base URL – you don’t need manual string interpolation.- If
$router.resolve()fails, you silently fall back to'', but the watcher will keep retrying.- const { domain } = this.$envConfig; + const { domain } = this.$envConfig || {}; ... - const resolved = this.$router.resolve({ + const { href } = this.$router.resolve({ name: 'LicenseeDetailPublic', params: { compact: compactType, licenseeId } }); - const urlObj = new URL(`${domain}${resolved.href}`); - url = urlObj.toString(); + url = new URL(href, domain).toString();Consider early–returning when
domainis missing, and log a warning to aid troubleshooting.
129-147: Cache CSS variable read to avoid repeated layout thrash
getComputedStyle(document.documentElement)is called every time a QR code is regenerated (watcher +mounted).
Move the colour lookup outside the function (module-level ormounted) and pass it in, or memoise the result.Also consider a secondary fallback (e.g., default palette constant) when the CSS var is absent.
webroot/src/pages/LicenseeProof/LicenseeProof.less (1)
161-185: Add responsive max-width to QR code image to avoid overflow on narrow viewportsWhen the QR code is rendered inside
.qr-code-section, extremely small mobile screens or print in portrait orientation may clip the image.
A quick fix:.qr-code-section { img { max-width: 100%; height: auto; } }This keeps the QR code legible without breaking layout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
webroot/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
webroot/package.json(2 hunks)webroot/src/components/Page/PageContainer/PageContainer.less(1 hunks)webroot/src/components/Page/PageContainer/PageContainer.ts(1 hunks)webroot/src/components/Page/PageContainer/PageContainer.vue(1 hunks)webroot/src/locales/en.json(1 hunks)webroot/src/locales/es.json(1 hunks)webroot/src/models/License/License.model.spec.ts(4 hunks)webroot/src/models/License/License.model.ts(1 hunks)webroot/src/pages/LicenseeProof/LicenseeProof.less(2 hunks)webroot/src/pages/LicenseeProof/LicenseeProof.ts(5 hunks)webroot/src/pages/LicenseeProof/LicenseeProof.vue(3 hunks)webroot/src/styles.common/_colors.less(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webroot/src/pages/LicenseeProof/LicenseeProof.ts (1)
webroot/src/models/License/License.model.ts (1)
License(67-189)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CheckWebroot
🔇 Additional comments (3)
webroot/src/locales/en.json (1)
687-690: Guard against missing translations in other locale files
publicProfileLinkandqrCodeAltwere only added to the English bundle here.
Please ensure the same keys exist in every supported locale (es.json, etc.) to prevent runtimeundefinedstrings when users switch languages.webroot/src/models/License/License.model.spec.ts (1)
132-134: Tests accurately reflect newdisplayNamebehaviourThe added assertions cover both default and abbreviated display modes across delimiters—good coverage. ✔️
Also applies to: 160-162, 218-219, 566-567
webroot/src/pages/LicenseeProof/LicenseeProof.vue (1)
62-63: Consistency: use full license type description everywhere
Figma spec calls for the full license type description (not abbreviation).license.displayName(', ')may still return the abbreviated version depending on model logic. Confirm the helper already switched to full names, otherwise users will still see short forms here.
|
@jlkravitz This is ready for your review. |
Requirements List
yarn install --ignore-enginesDescription List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsCloses #816
Summary by CodeRabbit
New Features
Improvements
Chores