Skip to content
This repository has been archived by the owner on Apr 5, 2022. It is now read-only.

Allow separate control of sharing in blog post and main menu #260

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

Conversation

fo2rist
Copy link
Contributor

@fo2rist fo2rist commented Jun 20, 2021

Description

  1. Use i18n title for share menu Update translation to match current view
  2. Make sharing from post body controlled separately from header menu

Motivation and Context

It was impossible to enable sharing from the main menu, but disable in the post.

Side note: Buttons in the post don't really match the light style of the theme, they're quite big and better be placed below the main content.

Screenshots (if appropriate):

The buttons are the same, but I'm not the expert in html/css. Maybe it's better to redesign them completely and make less obtrusive (e.g see reddit, it has a single small nice share button with pop-up menu).
Feel free to reach out to me in telegram if you want to discuss any of PRs in real-time

Checklist:

  • I have updated the documentation, as applicable.
  • I have updated the theme.toml, as applicable.

Update translation to match current view
- add parameter to control sharing from post body
  separately from header menu
- move sharing button to the bottom of the post and
  add title to make it clear it's sharing
@pacollins pacollins self-requested a review July 3, 2021 21:05
Copy link
Owner

@pacollins pacollins left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I like the idea, but maybe not this solution. It doesn't really solve the problem, it just moves them to where it bothers you less until the next person that comes along and wants them moved to the top.

I do agree that we could probably transition these icons to be more muted, or even drop them all for the share button that is in the header.

@@ -28,6 +28,7 @@ disableLanguages = []
imageStretch = ""
removeBlur = false
socialShare = ["twitter", "facebook", "reddit", "linkedin", "pinterest", "email"]
shareFromPost = true
Copy link
Owner

Choose a reason for hiding this comment

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

Changes to the config.toml should not change what legacy users expect if at all possible. Since this must be set as true, users that simply update the theme would be missing the share buttons if they didn't add this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to avoid this incompatibility with a new parameter?
The only one I can think of is to change it to disableSharingFromPost or disablePostSharing but will be somewhat inconsistent with all other parameters.
Let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layouts/_default/single.html Outdated Show resolved Hide resolved
@@ -37,7 +37,7 @@ <h1 class="nav-title">
{{ if .Site.Params.header.languageMenu }}{{ partial "language-menu" . }}{{ end }}
{{ if .Site.Params.header.shareMenu }}
<menu id="share-menu" class="flyout-menu menu">
<h1>Share Post</h1>
<h1>{{ i18n "share_post" }}</h1>
Copy link
Owner

Choose a reason for hiding this comment

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

Not quite sure why this wasn't i18n before, but we need to make sure its in all languages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've counted the number of occurrences of share_post in our codebase and the number of l10n files. Both are 13. The former should be strictly greater than the later coz this param is called in the Go-HTML template code. The -A <N> flag can be appended to grep to show <N> lines after the matched pattern.

$ find i18n -type f
i18n/fr.toml
i18n/zh-CN.toml
i18n/ja.toml
i18n/tr.toml
i18n/en.toml
i18n/ko.toml
i18n/id.toml
i18n/pl.toml
i18n/es.toml
i18n/nl.toml
i18n/de.toml
i18n/zh-TW.toml
i18n/pt.toml
$ find i18n -type f | wc -l
13
$ git grep share_post | wc -l
13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, the localized string was not used before. So I put the actual string as it appeared in browser to the i18n file. What's next? Do you want me to isolate this change from the share button update? Or you'll fix it? Or you want to start using the localized version but the longer from from the i18n/en?

i18n/en.toml Show resolved Hide resolved
Show sharing buttons when the config parameter is missing for backward compatibility
@fo2rist fo2rist changed the title Improve sharing buttons appearance Allow separate control of sharing in blog post and main menu Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants