-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
base: master
Are you sure you want to change the base?
Conversation
Updated config for exampleSite.
There was a problem hiding this 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}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-selected="true" | |
aria-selected="{{ eq $indexCompany 0 }}" |
id='{{ replace .company " " "-" }}-tab' | ||
data-bs-target='#pills-{{ replace .company " " "-" }}' | ||
aria-controls='{{ replace .company " " "-" }}' |
There was a problem hiding this comment.
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 " " "-" }}
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
id='pills-{{ replace .company " " "-" }}' | ||
aria-labelledby='pills-{{ replace .company " " "-" }}-tab' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<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 ) }} |
There was a problem hiding this comment.
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):
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 ) }} |
|
||
{{ .content | markdownify}} |
There was a problem hiding this comment.
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.
{{ .content | markdownify}} | |
<div class="pt-2"> | |
{{ .content | markdownify}} | |
</div> |
#experience .job-container { | ||
margin-top: 20px; | ||
} | ||
|
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<span class="p-2"> | |
<span class="p-2"> | |
{{ $jobStatus := cond (eq $indexJob 0) "Working" "Worked" }} |
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. |
Changed the experience section. This change allows to list multiple jobs per company in the same tab.