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

Icons showing shares when settings is None #81

Closed
JiveDig opened this issue May 24, 2019 · 3 comments
Closed

Icons showing shares when settings is None #81

JiveDig opened this issue May 24, 2019 · 3 comments

Comments

@JiveDig
Copy link

JiveDig commented May 24, 2019

Discussed with Bill already. I think i found the issue and will submit a PR in a min. Feel free to edit as needed.

Putting this here to reference the PR against.

@JiveDig
Copy link
Author

JiveDig commented May 24, 2019

Screenshot 2019-05-24 15 26 59

Screenshot 2019-05-24 15 27 28

@JiveDig
Copy link
Author

JiveDig commented Jul 19, 2019

Any chance of getting this merged/tested? Just about to launch the new theme (screenshot above) for them and hitting this on live.

@billerickson
Copy link
Collaborator

Sorry for the delay on this.

I'm having trouble reproducing the error. When I select "None" as the count and "Fancy" as the style, I don't get any counts displayed.

screenshot

There's no markup appearing for the count:

screenshot

The logic for removing the count markup is here. It's a bit confusing because both of these options reference settings that are hidden if you have "None" as your count source.

I was able to replicate your screenshot with the following workflow:

  • Go to Settings > Shared Counts
  • Select "SharedCount.com" as the count source
  • Uncheck "Count Total Only" and "Hide Empty Counts"
  • Select "None" as the count source
  • Click "Save Changes"

Rather than using CSS to hide the count if "None" is selected, I think we should extend the current approach to exclude the count markup if "None" is selected.

Can you test the issue/81 branch and see if it resolves the issue for you? Here's the relevant addition: 6d75ef1

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

Successfully merging a pull request may close this issue.

2 participants