-
Notifications
You must be signed in to change notification settings - Fork 134
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
New social buttons on navbar (#862) #863
Conversation
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.
Happy to see this PR.
Added a few thoughts and open to consider such a change with a few edits.
One issue design wise is the non existent space between the buttons. It needs a bit of space between the buttons. Could be added with something like ml-1
or so.
- id: linkedin.star | ||
translation: "Star" | ||
- id: linkedin.stars | ||
translation: "Stars" | ||
- id: twitter.star | ||
translation: "Star" | ||
- id: twitter.stars | ||
translation: "Stars" | ||
- id: facebook.star | ||
translation: "Star" | ||
- id: facebook.stars | ||
translation: "Stars" | ||
- id: custom.star | ||
translation: "Star" | ||
- id: custom.stars | ||
translation: "Stars" |
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.
Yeah button text should probably not be using the key "star" as this only makes sense for Github starring. Maybe a more neutral id: facebook.button_text
could work as well as the translation being being Facebook
.
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.
Same for the other languages.
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.
OK. I'll incorporate those changes as well.
<i class="{{ printf "%s" .icon | default (printf "fab fa-github") }} {{ if not .no_text }}mr-2 {{ end }}"></i> | ||
{{- if not .no_text -}} | ||
{{ .text | default (i18n "github.star") }} | ||
{{- end -}} | ||
</a> | ||
</div> | ||
{{- end }} | ||
{{- with .Params.twitter_button }} | ||
<div class="d-none d-lg-block pl-auto"> | ||
<a class="btn btn-light ignore-color-change" href="{{ .url }}" role="button" title="{{- i18n "navbar.twitter" | default "twitter" -}}"> | ||
<i class="{{ printf "%s" .icon | default (printf "fab fa-twitter") }} {{ if not .no_text }}mr-2 {{ end }}"></i> | ||
{{- if not .no_text -}} | ||
{{ .text | default (i18n "twitter.star") }} | ||
{{- end -}} | ||
</a> | ||
</div> | ||
{{- end }} | ||
{{- with .Params.facebook_button }} | ||
<div class="d-none d-lg-block pl-auto"> | ||
<a class="btn btn-light ignore-color-change" href="{{ .url }}" role="button" title="{{- i18n "navbar.facebook" | default "facebook" -}}"> | ||
<i class="{{ printf "%s" .icon | default (printf "fab fa-facebook") }} {{ if not .no_text }}mr-2 {{ end }}"></i> | ||
{{- if not .no_text -}} | ||
{{ .text | default (i18n "facebook.star") }} | ||
{{- end -}} | ||
</a> | ||
</div> | ||
{{- end }} | ||
{{- with .Params.linkedin_button }} | ||
<div class="d-none d-lg-block pl-auto"> | ||
<a class="btn btn-light ignore-color-change" href="{{ .url }}" role="button" title="{{- i18n "navbar.linkedin" | default "linkedin" -}}"> | ||
<i class="{{ printf "%s" .icon | default (printf "fab fa-linkedin") }} {{ if not .no_text }}mr-2 {{ end }}"></i> | ||
{{- if not .no_text -}} | ||
{{ .text | default (i18n "linkedin.star") }} | ||
{{- 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.
This is a bit manual and could be structured as a list. Any ideas on making it a list config wise as well as loop through the list in the code while keeping the backwards compatibility with the repo_button?
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'm really sorry for my lack of experience with Hugo templates. I'm quite new to Hugo templates. I actually started to mess with it yesterday using Syna as a way to better understand Hugo.
As you said, this patch is really to cumbersome to be useful so I'm planning to change it a little bit. In fact, today I notice that there is a better way to extend the template to make those icons more generic and, at same time keeping the backward compatibility with the existing repo_button.
My new plan is to left repo_button as is and add a new list of button that will add the new buttons in the same way repo_button works. This way we will be able to combine both repo_button and the new button options to keep backward compatibility or delegate the buttons to use only the new button instead. I'll also try to make shortcuts for the most common icons to make the usage easier.
I also notice that there are more places in the sample site that I must update in order to document the new features and I'm planning to update those locations as well.
Could you reject this pull request for now? As soon as I have a better solution I will send a new one. We are really glad to be able to contribute to Syna's main repository.
Thanks in advance!
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.
Sure no worries. Will close this one and wait for the next. Happy to help if anything comes up.
In terms of buttons that iterate take a look at this part of Syna: https://github.com/okkur/syna/blob/master/layouts/partials/fragments/buttons.html
Those are the call to action buttons, but it does showcase a more generic way of how it could work with Hugo.
What this PR does / why we need it:
Implements the features describe in the issue #862 by adding more social buttons in the nav bar.
Special notes for your reviewer:
We changed the example nav.md and the fragment nav.md to implement this issue. As a complement, I updated the English, French and Portuguese translations.
Release note: