Skip to content
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

Multiple jobs in one company #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hgokduman
Copy link

Changed the experience section. This change allows to list multiple jobs per company in the same tab.

Capture

Halim Gokduman (server) and others added 2 commits March 26, 2024 16:13
Copy link

@CarlosSanabriaM CarlosSanabriaM left a comment

Choose a reason for hiding this comment

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

@hgokduman Thank you so much for implementing this!

I needed this feature a lot :)

I think your implementation is really good, and not only implements the feature as expected, but also refactors the code by removing duplicate lines in the HTML of the experience.html file.

I have added some comments with some minor improvements and fixes. Could you please take a look to it?

@gurusabarish What do you think about this PR? Would it be possible to merge it after these changes have been made? Would be a great addition to your awesome theme!

Thanks to both for your awesome work!

{{ range $indexCompany, $company := .Site.Params.experience.companies }}
<li class="nav-item px-1 bg-transparent" role="presentation">
<div
class="nav-link bg-transparent{{ if (eq $indexCompany 0) }} active{{end}}"

Choose a reason for hiding this comment

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

Suggested change
class="nav-link bg-transparent{{ if (eq $indexCompany 0) }} active{{end}}"
class="nav-link bg-transparent{{ if (eq $indexCompany 0) }} active{{ end }}"

<li class="nav-item px-1 bg-transparent" role="presentation">
<div
class="nav-link bg-transparent{{ if (eq $indexCompany 0) }} active{{end}}"
aria-selected="true"

Choose a reason for hiding this comment

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

Suggested change
aria-selected="true"
aria-selected="{{ eq $indexCompany 0 }}"

Comment on lines +16 to +18
id='{{ replace .company " " "-" }}-tab'
data-bs-target='#pills-{{ replace .company " " "-" }}'
aria-controls='{{ replace .company " " "-" }}'

Choose a reason for hiding this comment

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

Define a variable to avoid code duplicity:

{{ $sanitizedCompany := replace .company " " "-" }}
Suggested change
id='{{ replace .company " " "-" }}-tab'
data-bs-target='#pills-{{ replace .company " " "-" }}'
aria-controls='{{ replace .company " " "-" }}'
id='{{ $sanitizedCompany }}-tab'
data-bs-target='#pills-{{ $sanitizedCompany }}'
aria-controls='{{ $sanitizedCompany }}'

Reference: https://gohugo.io/templates/introduction/#variables

Comment on lines +30 to +31
id='pills-{{ replace .company " " "-" }}'
aria-labelledby='pills-{{ replace .company " " "-" }}-tab'

Choose a reason for hiding this comment

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

Suggested change
id='pills-{{ replace .company " " "-" }}'
aria-labelledby='pills-{{ replace .company " " "-" }}-tab'
id='pills-{{ $sanitizedCompany }}'
aria-labelledby='pills-{{ $sanitizedCompany }}-tab'

<div class="tab-content pb-5 pt-2 bg-transparent primary-font" id="pills-tabContent">
{{ range $indexCompany, $company := .Site.Params.experience.companies }}
<div
class="tab-pane fade bg-transparent{{ if (eq $indexCompany 0) }} show active{{end}}"

Choose a reason for hiding this comment

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

Suggested change
class="tab-pane fade bg-transparent{{ if (eq $indexCompany 0) }} show active{{end}}"
class="tab-pane fade bg-transparent{{ if (eq $indexCompany 0) }} show active{{ end }}"

aria-labelledby='pills-{{ replace .company " " "-" }}-tab'
>
{{ range $indexJob, $job := .jobs }}
<div class="job-container{{if (eq $indexJob 0) }}-first{{end}}">

Choose a reason for hiding this comment

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

Suggested change
<div class="job-container{{if (eq $indexJob 0) }}-first{{end}}">
<div class="job-container{{ if (eq $indexJob 0) }}-first{{ end }}">

<span class="h4">{{ .job }}</span>
<small>-</small>
<a href="{{ $company.companyUrl }}" target="_blank">{{ $company.company }}</a>
<div class="pb-1">

Choose a reason for hiding this comment

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

Suggested change
<div class="pb-1">
<div class="pb-1">

style="cursor: pointer;"
data-bs-toggle="tooltip"
data-bs-placement="top"
data-bs-original-title={{ .info.content | default (print "Working as a " .job " at " $company.company ) }}
Copy link

@CarlosSanabriaM CarlosSanabriaM Nov 7, 2024

Choose a reason for hiding this comment

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

This needs to be fixed in order to keep previous behaviour (show Working when job is current and Worked when not):

Suggested change
data-bs-original-title={{ .info.content | default (print "Working as a " .job " at " $company.company ) }}
data-bs-original-title={{ .info.content | default (print $jobStatus " as a " .job " at " $company.company ) }}

Comment on lines +63 to +64

{{ .content | markdownify}}

Choose a reason for hiding this comment

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

I think the code in the main branch has a bug related to this. The 1st tab (current job) doesn't include the top padding before the job content, whereas the rest include it. I think the intention was to always include this padding.

Suggested change
{{ .content | markdownify}}
<div class="pt-2">
{{ .content | markdownify}}
</div>
No padding

no-padding

Padding

padding

Comment on lines +387 to +390
#experience .job-container {
margin-top: 20px;
}

Choose a reason for hiding this comment

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

I think this should be done with bootstrap instead of with CSS.

Something similar to this:

<div class="pt-2">

To include top padding. What do you think?

<div class="pb-1">
<small>{{ .date }}</small>
{{ if .info.enable | default true }}
<span class="p-2">

Choose a reason for hiding this comment

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

Suggested change
<span class="p-2">
<span class="p-2">
{{ $jobStatus := cond (eq $indexJob 0) "Working" "Worked" }}

@CarlosSanabriaM
Copy link

@gurusabarish @hgokduman

I have applied all the changes mentioned above in the following PR: #203

I think it's easier to merge that one instead, and close this one.

All the credits to @hgokduman for the awesome work. I just applied the mentioned changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants