-
Notifications
You must be signed in to change notification settings - Fork 130
Share button in the Featured Circuit Card #417
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
Share button in the Featured Circuit Card #417
Conversation
WalkthroughThis change introduces two new localization string keys— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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: 2
🧹 Nitpick comments (3)
lib/l10n/app_en.arb (2)
378-379: Improve wording & follow existing style guide for hints
The phrase"Enter your Project Tags divided by comma[,]"is grammatically awkward, differs from the earlier"…separated by comma"pattern (see Line 363), and hard-codes the comma symbol redundantly inside[,].- "edit_project_tag_hint": "Enter your Project Tags divided by comma[,]", + "edit_project_tag_hint": "Enter project tags separated by comma",• Keeps wording consistent with other hints
• Drops unnecessary capital letters and[,]marker
• Easier to translate downstream
380-382: Inconsistent casing & missing metadata entry
"featured_projects_card_view": "VIEW"is all caps while similar keys (group_member_card_view,assignment_card_view, etc.) use"View". This breaks visual consistency in the UI.- "featured_projects_card_view": "VIEW", + "featured_projects_card_view": "View",Also consider adding a
@featured_projects_card_viewmetadata block with a short description to aid translators:"@featured_projects_card_view": { "description": "Button label on the Featured Project card that opens the project" },lib/ui/views/projects/components/featured_project_card.dart (1)
100-105: Good implementation but consider URL construction consistency.The
_getProjectUrlmethod constructs a shareable embed URL properly. However, this logic duplicates the URL base construction pattern from_getImageUrl()method.Consider extracting the base URL construction into a private helper method to reduce duplication:
+ String _getBaseUrl() { + return Uri.parse(EnvironmentConfig.CV_API_BASE_URL) + .replace(pathSegments: []) + .toString() + .replaceAll(RegExp(r'\/+$'), ''); + } + String _getProjectUrl() { - final base = Uri.parse( - EnvironmentConfig.CV_API_BASE_URL, - ).replace(pathSegments: []).toString().replaceAll(RegExp(r'\/+$'), ''); - return '$base/simulator/embed/${widget.project.id}'; + return '${_getBaseUrl()}/simulator/embed/${widget.project.id}'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/gen_l10n/app_localizations.dart(1 hunks)lib/gen_l10n/app_localizations_ar.dart(1 hunks)lib/gen_l10n/app_localizations_en.dart(1 hunks)lib/gen_l10n/app_localizations_hi.dart(1 hunks)lib/l10n/app_ar.arb(1 hunks)lib/l10n/app_en.arb(1 hunks)lib/l10n/app_hi.arb(1 hunks)lib/ui/views/projects/components/featured_project_card.dart(3 hunks)lib/ui/views/projects/edit_project_view.dart(6 hunks)lib/ui/views/projects/project_details_view.dart(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: lib/gen_l10n/app_localizations_en.dart:797-811
Timestamp: 2025-06-17T13:37:11.440Z
Learning: When fixing localization issues in Flutter, always fix the source ARB files (like app_en.arb, app_hi.arb) rather than the generated Dart files (like app_localizations_en.dart). The generated localization files are auto-generated from ARB files and any manual changes to them will be overwritten when the localization files are regenerated.
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: lib/gen_l10n/app_localizations_en.dart:766-796
Timestamp: 2025-06-17T13:38:03.098Z
Learning: In Flutter localization, ARB files (like app_en.arb) are the source files that get compiled into generated localization classes (like app_localizations_en.dart). Issues with incorrect language content should be fixed in the source ARB files rather than the generated files.
📚 Learning: in flutter localization, arb files (like app_en.arb) are the source files that get compiled into gen...
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: lib/gen_l10n/app_localizations_en.dart:766-796
Timestamp: 2025-06-17T13:38:03.098Z
Learning: In Flutter localization, ARB files (like app_en.arb) are the source files that get compiled into generated localization classes (like app_localizations_en.dart). Issues with incorrect language content should be fixed in the source ARB files rather than the generated files.
Applied to files:
lib/l10n/app_ar.arblib/l10n/app_en.arblib/gen_l10n/app_localizations_ar.dartlib/l10n/app_hi.arblib/gen_l10n/app_localizations.dartlib/gen_l10n/app_localizations_en.dart
📚 Learning: when fixing localization issues in flutter, always fix the source arb files (like app_en.arb, app_hi...
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: lib/gen_l10n/app_localizations_en.dart:797-811
Timestamp: 2025-06-17T13:37:11.440Z
Learning: When fixing localization issues in Flutter, always fix the source ARB files (like app_en.arb, app_hi.arb) rather than the generated Dart files (like app_localizations_en.dart). The generated localization files are auto-generated from ARB files and any manual changes to them will be overwritten when the localization files are regenerated.
Applied to files:
lib/l10n/app_ar.arblib/l10n/app_en.arblib/gen_l10n/app_localizations_ar.dartlib/l10n/app_hi.arblib/gen_l10n/app_localizations_hi.dartlib/gen_l10n/app_localizations.dartlib/gen_l10n/app_localizations_en.dart
🔇 Additional comments (18)
lib/gen_l10n/app_localizations_hi.dart (1)
1075-1081: Don't commit manual edits to generated localization filesThe two new getters look correct, but remember that any direct change under
lib/gen_l10n/will be overwritten the next timeflutter gen-l10nruns.
Double-check that the real source ARB file (lib/l10n/app_hi.arb) already contains the keys"edit_project_tag_hint"and"featured_projects_card_view"; if not, move the strings there and re-generate.lib/gen_l10n/app_localizations_en.dart (1)
1067-1073: Verify ARB source before mergingAs with the Hindi file, these getters should come only from the generator. Please confirm that
app_en.arbwas updated and committed; otherwise this addition will disappear on the next localization build.Also, consider re-phrasing the hint to
Enter your project tags separated by commas (,)for slightly clearer English.lib/l10n/app_ar.arb (1)
379-382: LGTM! Arabic localization additions are correctly formatted.The new localization keys are properly structured with appropriate Arabic translations:
edit_project_tag_hintprovides clear instructions for tag inputfeatured_projects_card_viewcorrectly translates "View"- Metadata entry follows ARB format conventions
lib/l10n/app_hi.arb (1)
376-379: LGTM! Hindi localization additions are correctly implemented.The new localization keys follow proper ARB format and provide accurate Hindi translations:
edit_project_tag_hintgives clear guidance for tag input with proper Hindi translationfeatured_projects_card_viewcorrectly translates to "देखें" (View)- Metadata entry is properly formatted
lib/ui/views/projects/components/featured_project_card.dart (4)
6-7: LGTM! Appropriate imports for new functionality.The
share_pluspackage import enables the sharing functionality, andapp_localizationsimport supports the localization of the VIEW button text.
107-115: Well-implemented sharing functionality with proper error handling.The
_shareProjectmethod correctly uses theshare_pluspackage and includes appropriate error handling with debug logging. The async implementation is proper.
199-209: Excellent addition of share functionality with good UX.The share IconButton is well-positioned, properly styled to match the design system, and includes a helpful tooltip. The placement next to the VIEW button creates a logical action group.
216-218: Perfect localization implementation.The replacement of the hardcoded "VIEW" string with
AppLocalizations.of(context)!.featured_projects_card_viewproperly internationalizes the UI. This aligns with the localization keys added in the ARB files.lib/ui/views/projects/project_details_view.dart (4)
234-235: LGTM! Improved font size consistency.The font size reduction from 18 to 16 creates better visual hierarchy and consistency with other UI elements.
239-240: LGTM! Consistent font sizing.The HTML body font size now matches the header, creating visual consistency in the project description section.
725-725: LGTM! Logical placement of tags widget.The tags display is appropriately positioned after the project description, following a natural information hierarchy.
247-296: Localization key verified and code approvedI’ve confirmed that
edit_project_tags_listis defined in all ARB files (app_en.arb,app_ar.arb,app_hi.arb) with appropriate translations. The_buildProjectTags()and_buildTagChip()methods are well-structured, handle empty states correctly, and follow the app’s design system and localization practices.– No further changes needed.
lib/ui/views/projects/edit_project_view.dart (6)
34-34: Excellent form state management improvement.The addition of
TextEditingControllerinstances for name and tags fields, along with proper disposal, provides better control over form state compared to usinginitialValue. The resource cleanup indispose()prevents memory leaks.Also applies to: 39-41, 47-49
58-67: LGTM! Proper controller initialization.The controllers are correctly initialized with existing project data, and the QuillController properly handles existing description content. The lazy initialization pattern is appropriate.
72-72: LGTM! Consistent controller usage.Using
_nameControllerinstead ofinitialValueis consistent with the improved form state management approach.
114-175: Excellent localization implementation for access types.The comprehensive localization of the access type dropdown is well-implemented with:
- Clean separation between internal API values and localized display strings
- Proper two-way conversion via
getCurrentValue()helper function- Consistent handling of all access type options
This follows internationalization best practices and maintains API compatibility.
191-192: LGTM! Proper form value capture.Using controller values directly in form submission ensures the current state is properly captured, which is the correct approach when using
TextEditingController.
87-110: Localization key verified and UX improvement approved
- Confirmed that
edit_project_tag_hintexists in all ARB files underlib/l10n/:
- app_en.arb
- app_hi.arb
- app_ar.arb
- No further changes needed. This hint text enhancement can be merged as is.
Pull Request Test Coverage Report for Build 16757055814Details
💛 - Coveralls |
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Fixes #389
Describe the changes you have made in this PR -
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Improvements
UI Updates