-
Notifications
You must be signed in to change notification settings - Fork 9
[LC-1081] Properly handle alignments field #799
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ef94948 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for learncarddocs canceled.
|
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.
✨ PR Review
The PR properly implements the change from 'alignment' to 'alignments' field throughout the codebase. The changes are consistent across components and templates, with appropriate type updates and test data additions.
1 issues detected:
🧹 Maintainability - Code is removed and immediately re-added in identical form, creating unnecessary version history noise.
Details: The address field code is removed and then immediately re-added in the exact same form, creating unnecessary diff noise without functional changes.
File:packages/plugins/vc-templates/src/templates.ts
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
🥷 Code experts: gerardopar, TaylorBeeston gerardopar, TaylorBeeston have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ 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.
✨ PR Review
The PR properly implements the transition from singular alignment
to plural alignments
field to comply with Open Badges v3 specification. The changes are consistent across the codebase and include proper test data updates.
2 issues detected:
🧾 Readability - Type and component names should follow consistent naming conventions for better code readability and maintainability.
Details: The type interface is named
AlignmentsRowProps
while the component function is namedAlignmentRow
, creating inconsistent naming convention that differs from the CertificateDisplayCard version which usesAlignmentRowProps
.
File:packages/react-learn-card/src/components/MeritBadgeDisplayCard/AlignmentRow.tsx (4-4)
🧹 Maintainability - Duplicated code across multiple components increases maintenance burden and risk of inconsistent updates.
Details: The AlignmentRow component in MeritBadgeDisplayCard is identical to the one in CertificateDisplayCard, creating unnecessary code duplication that increases maintenance overhead and potential for inconsistencies.
File:packages/react-learn-card/src/components/MeritBadgeDisplayCard/AlignmentRow.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
import React from 'react'; | ||
import CaretRightFilled from '../../assets/images/CaretRightFilled.svg'; | ||
|
||
type AlignmentsRowProps = { |
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.
🧾 Readability - Inconsistent Naming: Rename the type from AlignmentsRowProps
to AlignmentRowProps
to maintain consistency with the component name and match the pattern used in the CertificateDisplayCard version.
type AlignmentsRowProps = { | |
type AlignmentRowProps = { |
Overview
🎟 Relevant Jira Issues
LC-1081
📚 What is the context and goal of this PR?
We had some handling for the
alignments
field, but it wasn't quite right. So this is mostly just changing the field name fromalignment
toalignments
. It also adds updates some types and adds thealignments
field to the storybookAllFieldsCredential
test credential🥴 TL; RL:
Properly handle Obv3
alignments
field💡 Feature Breakdown (screenshots & videos encouraged!)
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
pnpm storybook
(from LearnCard dir)📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📜 Gitbook
📊 Storybook
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Add OBv3 alignments field support to VC templates and React components to properly display credential alignments.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀