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

Fix 3484: hide the share toggle button if no service is enabled #3492

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@bdunogier
Copy link
Contributor

bdunogier commented Dec 12, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? aye
Documentation no
Translation no
CHANGELOG.md no
Fixed tickets #3484
License MIT

Hides the "Share" toolbar entry completely if all the share services are disabled.

I'm not really happy with the big "if" block, but I'd say it'd be overdoing it to change it without having a more modulare sharing system (e.g. be able to add/remove supported services). The list is hardcoded in the template, it can be hardcoded in the condition for it.

@bdunogier bdunogier force-pushed the bdunogier:fix-3484-disable_share_link branch from 98a28ca to 0ef1ffb Dec 12, 2017

@j0k3r j0k3r added this to the 2.3.1 milestone Dec 12, 2017

@j0k3r j0k3r requested review from nicosomb , Kdecherf and tcitworld Dec 12, 2017

@j0k3r

j0k3r approved these changes Dec 12, 2017

Copy link
Member

j0k3r left a comment

LGTM

@nicosomb

This comment has been minimized.

Copy link
Member

nicosomb commented Dec 12, 2017

It looks OK for me.
If you want to improve that, you can maybe create a Twig function. What do you think?

@bdunogier

This comment has been minimized.

Copy link
Contributor

bdunogier commented Dec 12, 2017

Yeah, it would be one way to do it. As said in the description, I think it's fine as it is. But you're the maintainers, you tell me :)

@nicosomb

This comment has been minimized.

Copy link
Member

nicosomb commented Dec 12, 2017

If you can take some time to do it and if you want to do it, let's go. I can merge right now if you prefer ;-)

@nicosomb nicosomb merged commit d80e993 into wallabag:master Dec 12, 2017

4 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
zappr/pr/specification PR has passed specification checks
zappr/pr/tasks PR has no open tasks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment