Feature/lfx 1843 work history #2716
Conversation
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a new timeline format for displaying work history in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
frontend/src/modules/contributor/components/details/contributor-details-work-history.vue (2)
94-95: Consider replacinglodash'sgroupBywith native JavaScript.Using
lodash'sgroupByfunction adds an external dependency. You can achieve the same functionality with native JavaScript, which can help reduce bundle size and improve performance.Here's how you can use native JavaScript to group the organizations:
const grouped = props.contributor.organizations.reduce((acc, organization) => { const key = organization.id; if (!acc[key]) { acc[key] = []; } acc[key].push(organization); return acc; }, {});
130-137: Simplify the logic inminimumShownGroupscomputation.The computation for
minimumShownGroupsis somewhat complex and might be hard to understand at first glance. Simplifying it or adding comments can improve code readability and maintainability.Consider this simplified version:
const minimumShownGroups = computed(() => { const MIN_ITEMS = 3; return Math.min(orgGrouped.value.length, MIN_ITEMS); });If more complex logic is necessary, adding explanatory comments would be beneficial.
frontend/src/ui-kit/timeline/Timeline.vue (3)
2-2: Consider adding aria-label for accessibilityThe timeline structure should be more accessible to screen readers.
- <div v-for="group in props.groups" :key="group.label" class="c-timeline"> + <div v-for="group in props.groups" :key="group.label" class="c-timeline" role="list" aria-label="Timeline">
13-24: Add title attribute for truncated textSince the group label might be truncated in some cases (as seen in the story), it would be helpful to show the full text on hover.
- {{ group.label }} + <span :title="group.label">{{ group.label }}</span>
37-39: Consider type-safe props definitionThe props interface could be more specific about the expected types.
-const props = defineProps<{ - groups: TimelineGroup[]; -}>(); +interface TimelineProps { + groups: TimelineGroup[]; +} +const props = defineProps<TimelineProps>();frontend/src/ui-kit/timeline/Timeline.stories.ts (1)
20-27: Improve readability of sample dataThe sample data is too condensed and hard to read. Consider formatting it for better readability.
- groups: [ - { - label: 'Group 1', labelLink: { name: 'organizationView', params: { id: '1' } }, icon: 'https://avatars.githubusercontent.com/u/38015056?v=4', items: [{ id: 1, label: 'Item 1', date: 'Aug 2024 - Dec 2024' }, { id: 2, label: 'Item 2', date: 'Aug 2024' }, { id: 3, label: 'Item 3', date: 'Aug 2024' }, { id: 4, label: 'Item 4', date: 'Aug 2024' }], - }, - { - label: 'Group 2', labelLink: { name: 'organizationView', params: { id: '2' } }, icon: 'https://avatars.githubusercontent.com/u/38015056?v=4', items: [{ id: 3, label: 'Item 3', date: 'Aug 2024' }, { id: 4, label: 'Item 4', date: 'Aug 2024' }], - }, - ], + groups: [ + { + label: 'Group 1', + labelLink: { name: 'organizationView', params: { id: '1' } }, + icon: 'https://avatars.githubusercontent.com/u/38015056?v=4', + items: [ + { id: 1, label: 'Item 1', date: 'Aug 2024 - Dec 2024' }, + { id: 2, label: 'Item 2', date: 'Aug 2024' }, + { id: 3, label: 'Item 3', date: 'Aug 2024' }, + { id: 4, label: 'Item 4', date: 'Aug 2024' } + ], + }, + { + label: 'Group 2', + labelLink: { name: 'organizationView', params: { id: '2' } }, + icon: 'https://avatars.githubusercontent.com/u/38015056?v=4', + items: [ + { id: 3, label: 'Item 3', date: 'Aug 2024' }, + { id: 4, label: 'Item 4', date: 'Aug 2024' } + ], + }, + ],frontend/src/modules/contributor/components/details/work-history/contributor-details-work-history-item.vue (3)
Line range hint
7-14: Improve accessibility of truncated textThe truncated text with hardcoded max-width might not be responsive and accessible.
- <p class="truncate" style="max-width: 30ch"> + <p class="truncate max-w-[30ch]" :title="props.organization?.memberOrganizations?.title">
Line range hint
19-40: Enhance dropdown menu accessibilityThe dropdown menu is only accessible via hover, which may cause issues for keyboard and screen reader users.
- <lf-dropdown v-if="hovered" placement="bottom-end" width="14.5rem"> + <lf-dropdown + v-if="hovered" + placement="bottom-end" + width="14.5rem" + role="menu" + aria-label="Work experience actions" + > <template #trigger> - <lf-button type="secondary-ghost" size="small" :icon-only="true"> + <lf-button + type="secondary-ghost" + size="small" + :icon-only="true" + aria-label="More actions" + @keydown.enter="hovered = true" + >
Line range hint
92-106: Consider extracting date formatting logicThe date formatting logic could be moved to a separate utility function for reusability and easier testing.
Consider creating a new file
utils/date-formatter.ts:export const formatDateRange = (dateStart?: string, dateEnd?: string, format: string = 'MMMM YYYY'): string => { const start = dateStart ? moment(dateStart).utc().format(format) : 'Unknown'; const endDefault = dateStart ? 'Present' : 'Unknown'; const end = dateEnd ? moment(dateEnd).utc().format(format) : endDefault; return start === end ? start : `${start} → ${end}`; };Then update the component to use this utility:
-const getDateRange = (dateStart?: string, dateEnd?: string) => { - const start = dateStart - ? moment(dateStart).utc().format('MMMM YYYY') - : 'Unknown'; - const endDefault = dateStart ? 'Present' : 'Unknown'; - const end = dateEnd - ? moment(dateEnd).utc().format('MMMM YYYY') - : endDefault; - if (start === end) { - return start; - } - return `${start} → ${end}`; -}; +import { formatDateRange } from '@/utils/date-formatter'; +const getDateRange = formatDateRange;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
frontend/src/modules/contributor/components/details/contributor-details-work-history.vue(3 hunks)frontend/src/modules/contributor/components/details/work-history/contributor-details-work-history-item.vue(1 hunks)frontend/src/ui-kit/timeline/Timeline.stories.ts(1 hunks)frontend/src/ui-kit/timeline/Timeline.vue(1 hunks)frontend/src/ui-kit/timeline/TimelineItem.vue(1 hunks)frontend/src/ui-kit/timeline/timeline.scss(1 hunks)frontend/src/ui-kit/timeline/types/TimelineTypes.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/ui-kit/timeline/timeline.scss (1)
1-38: Well-structured SCSS using Tailwind CSS utilities.
The SCSS for the timeline component is organized and effectively utilizes Tailwind CSS utility classes for styling. This approach maintains consistency and simplifies maintenance.
frontend/src/modules/contributor/components/details/contributor-details-work-history.vue
Outdated
Show resolved
Hide resolved
joanagmaia
left a comment
joanagmaia
left a comment
There was a problem hiding this comment.
Hey Efren 👋
I've listed most of the issues in the PR, but also left a video if it helps a bit more
Let me know if you have any questions
https://www.loom.com/share/2c68c000858f42e6851b521978a61b60?sid=ca64433b-8abb-4d13-b8ce-3a29120be4bc
frontend/src/modules/contributor/components/details/contributor-details-work-history.vue
Outdated
Show resolved
Hide resolved
| <p class="truncate" style="max-width: 30ch"> | ||
| {{ item.label }} | ||
| </p> |
There was a problem hiding this comment.
This should have by default a text-gray-900 color, and only on hover a blue one (link behaviour)
There was a problem hiding this comment.
This is just a sample content inside the timeline component. You can theoretically use anything inside the content of the lf-timeline-item. I've added the text-gray-900 anyway.
| })); | ||
| }); | ||
| const minimumShownGroups = computed(() => { | ||
| const MIN_ITEMS = 3; |
There was a problem hiding this comment.
Can you confirm that this logic is working? If you check the video I have 3 organizations but only 2 are showing.
By default we should always show 3 organizations regardless of how many titles we have for each one of them
There was a problem hiding this comment.
I sent a message through slack regarding this. I reverted this anyway to the old fixed 3 organization limit regardless of how many roles the contributor had in that org.
| </router-link> | ||
|
|
||
| <div class="flex flex-auto flex-col overflow-hidden"> | ||
| <div v-if="props.organization?.memberOrganizations?.title" class="text-small text-gray-500 mb-1.5 flex items-center gap-1.5"> |
There was a problem hiding this comment.
This color should be updated to text-gray-900
frontend/src/modules/contributor/components/details/contributor-details-work-history.vue
Show resolved
Hide resolved
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
…v/crowd.dev into feature/LFX-1843-work-history
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
|
@joanagmaia I've addressed the items you highlighted |
joanagmaia
left a comment
joanagmaia
left a comment
There was a problem hiding this comment.
All good. Just one last fix:
We shouldn't exceed the width of that section.
Check image 2 - "..." button is vertically aligned with "+" button.
Check image 1 - since job title is big, the "..." button is overflowing the section, there's also no space between the title and the button
So we should:
- Have a gap space between title and button
- truncate job title to prevent the item from overflowing the section
|
@joanagmaia I've pushed an update to fix that last bit you highlighted. |
Changes proposed ✍️
What
Implemented a new timeline UI for the contributor's work history based on this design: https://www.figma.com/design/IKlC7mpFdr0GtiSH182KIX/1.-LFX-CM-%7C-Product---Core?node-id=1571-37749&node-type=frame&t=6kxre7hEcRK031S6-0
Why
To improve the work history section, we will display the organizations and roles similar to what LinkedIn does.
Ticket: https://linear.app/lfx/issue/LFX-1843/improve-work-history-timeline
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
LfTimelineandLfTimelineItem.Bug Fixes
Style
Documentation
LfTimelinecomponent to showcase its usage.