feat: Add speaker component with Content Fragment support and update filters#11
feat: Add speaker component with Content Fragment support and update filters#11tusharmane-tm merged 1 commit intomainfrom
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits
|
Summary of ChangesHello @tusharmane-tm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new 'Speaker' component, enhancing the system's ability to present speaker profiles. The component is designed to be highly flexible by leveraging Content Fragments for data sourcing, which streamlines content management and ensures reusability. The changes involve structural updates to component configuration files to properly register and define the new component and its data model. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'speaker' component with support for fetching content from Content Fragments. The changes include updating the component definitions, models, and filters in the main JSON configuration files. New JavaScript and CSS files for the speaker block functionality and styling are also included.
My review has identified a critical bug in the fallback logic for the Content Fragment loading, which will cause a runtime error. I've also found a high-severity issue where HTML content is not rendered correctly. Additionally, there are several opportunities for code improvements regarding code duplication, dead code, and unclear implementation of social links. I've also noted a potential configuration duplication issue that could affect maintainability. Please see the detailed comments for suggestions.
| { | ||
| "title": "Speaker", | ||
| "id": "speaker", | ||
| "plugins": { | ||
| "xwalk": { | ||
| "page": { | ||
| "resourceType": "core/franklin/components/block/v1/block", | ||
| "template": { | ||
| "name": "Speaker", | ||
| "model": "speaker" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
While the new 'Speaker' component definition is functionally correct, it seems to duplicate the configuration found in the new file ue/models/blocks/_speaker.json. This can lead to maintenance issues if the two definitions diverge. If this duplication is not part of an automated build process, consider consolidating them into a single source of truth to improve maintainability.
| { | ||
| "id": "speaker", | ||
| "fields": [ | ||
| { | ||
| "component": "aem-content", | ||
| "name": "fragmentPath", | ||
| "label": "Content Fragment Path", | ||
| "description": "Path to the Content Fragment containing speaker data" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
The new model for the 'speaker' component appears to be a duplicate of the model defined in ue/models/blocks/_speaker.json. Maintaining configuration in multiple places can be error-prone. If possible, it would be better to have a single source of truth for component models to avoid inconsistencies in the future.
1 participant
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Fix #
Test URLs: