Skip to content

fix: adjust recommendation designation visibility #110

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 8, 2025
Merged

fix: adjust recommendation designation visibility #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/partials/RecommendationPartial.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
<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">
Copy link

@gemini-code-assist gemini-code-assist bot Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are a couple of improvements for this line:

  1. Robustness of v-if: The current condition v-if="item.person.designation" will be true for a string containing only whitespace (e.g., " "). This would render an empty div, which could create unwanted vertical space because of the space-y-1 on the parent element. It's better to trim the string before checking for its truthiness.

  2. Testability: To make testing more robust and less coupled to implementation details like CSS classes, it's a good practice to add a data-testid attribute. 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">

{{ item.person.designation }}
</div>
<div class="flex justify-between text-xs dark:text-teal-500 text-slate-400 pb-2">
<div>{{ item.relation }}</div>
<div>{{ item.formattedDate }}</div>
Expand Down
20 changes: 20 additions & 0 deletions tests/partials/RecommendationPartial.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,25 @@ describe('RecommendationPartial', () => {
expect(renderMarkdown).toHaveBeenCalledWith('**great**');
expect(wrapper.html()).toContain('<strong>great</strong>');
expect(wrapper.text()).toContain('now');
expect(wrapper.text()).toContain(data[0].person.designation);
Copy link

@gemini-code-assist gemini-code-assist bot Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);

});

it('does not render designation markup when missing', () => {
const wrapper = mount(RecommendationPartial, {
props: {
recommendations: [
{
...data[0],
person: {
...data[0].person,
designation: '',
},
},
],
backToTopTarget: '#top',
},
});

expect(wrapper.html()).not.toContain('text-sm text-slate-600 dark:text-slate-300');
Copy link

@gemini-code-assist gemini-code-assist bot Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);

});
});
Loading