fix: adjust recommendation designation visibility #110
Conversation
Summary of ChangesHello @gocanto, 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 PR addresses a display issue in the recommendation partial by making the 'designation' field conditionally visible and applying consistent styling. It also includes new tests to cover the scenario where the designation is missing. Highlights
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
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds conditional rendering of a person’s designation in RecommendationPartial.vue, displaying it beneath the company when present. Updates tests to assert inclusion when designation exists and absence of the designation markup when it’s empty. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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. Comment |
gocanto
deleted the
gocanto/add-designation-to-recommendation-item-2025-10-08
branch
![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
Thank you for this contribution. The changes correctly implement the conditional rendering of the recommender's designation. I've added a few suggestions to improve the robustness of the implementation and the associated tests, mainly by making the conditional check stricter and using data-testid attributes for more resilient testing.
| <div class="pl-20 space-y-1"> | ||
| <div class="font-aspekta font-[650] text-slate-800 dark:text-slate-100">{{ item.person.full_name }}</div> | ||
| <div class="text-sm font-medium text-slate-800 dark:text-slate-100">{{ item.person.company }}</div> | ||
| <div v-if="item.person.designation" class="text-sm text-slate-600 dark:text-slate-300"> |
There was a problem hiding this comment.
There are a couple of improvements for this line:
-
Robustness of
v-if: The current conditionv-if="item.person.designation"will be true for a string containing only whitespace (e.g.," "). This would render an emptydiv, which could create unwanted vertical space because of thespace-y-1on the parent element. It's better to trim the string before checking for its truthiness. -
Testability: To make testing more robust and less coupled to implementation details like CSS classes, it's a good practice to add a
data-testidattribute. This allows tests to select this element without relying on fragile selectors like CSS classes.
I've combined both improvements in the suggestion below. You may also consider adding a test case for a designation containing only whitespace to ensure it's handled correctly.
<div v-if="item.person.designation.trim()" data-testid="recommendation-designation" class="text-sm text-slate-600 dark:text-slate-300">
| expect(renderMarkdown).toHaveBeenCalledWith('**great**'); | ||
| expect(wrapper.html()).toContain('<strong>great</strong>'); | ||
| expect(wrapper.text()).toContain('now'); | ||
| expect(wrapper.text()).toContain(data[0].person.designation); |
There was a problem hiding this comment.
While wrapper.text().toContain(...) works, it can be fragile. If the designation text were to appear elsewhere in the component for any reason, this test could pass incorrectly. A more specific assertion using a data-testid would be more robust and precise.
This change assumes you've added data-testid="recommendation-designation" to the designation div in RecommendationPartial.vue as suggested in my other comment.
const designationEl = wrapper.find('[data-testid="recommendation-designation"]');
expect(designationEl.exists()).toBe(true);
expect(designationEl.text()).toBe(data[0].person.designation);| }, | ||
| }); | ||
|
|
||
| expect(wrapper.html()).not.toContain('text-sm text-slate-600 dark:text-slate-300'); |
There was a problem hiding this comment.
This assertion is brittle because it's tied to the CSS classes. If the styling changes, this test will break even if the component's logic is still correct. It's better to use a data-testid selector to check for the element's existence. This makes the test more robust and maintainable.
This change assumes you've added data-testid="recommendation-designation" to the designation div in RecommendationPartial.vue as suggested in my other comment.
expect(wrapper.find('[data-testid="recommendation-designation"]').exists()).toBe(false);
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e64e37e7a08333b4eb20af7b25a6f7
Summary by CodeRabbit
New Features
Tests