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

Social share from the asset page #801

Merged
merged 21 commits into from Feb 13, 2019
Merged

Social share from the asset page #801

merged 21 commits into from Feb 13, 2019

Conversation

rstorey
Copy link
Member

@rstorey rstorey commented Feb 4, 2019

Refs #475

@coveralls
Copy link

coveralls commented Feb 5, 2019

Coverage Status

Coverage increased (+0.1%) to 69.694% when pulling f191c38 on social-share-asset into c2a10f9 on master.

concordia/static/css/base.css Outdated Show resolved Hide resolved
<div class="btn-group btn-group-sm" role="navigation" aria-label="Links to share this page on Facebook and Twitter">
<a class="btn btn-default" target="_blank" role="button" title="Share this page on Facebook" href="https://www.facebook.com/share.php?u={{ share_url }}"><span class="facebook-icon"></span></a>
<a class="btn btn-default" target="_blank" role="button" title="Share this page on Twitter" href="https://twitter.com/intent/tweet?text={{ tweet_text }}&source=webclient"><span class="twitter-icon"></span></a>
<a class="btn btn-default" id="copy-url-button" role="button" data-toggle="tooltip" data-trigger="click" data-placement="bottom" title="Copied URL to your clipboard"><i class="fas fa-link white"></i></a>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be “Copy URL…” to match the previous two button's styles when read aloud?

Suggested change
<a class="btn btn-default" id="copy-url-button" role="button" data-toggle="tooltip" data-trigger="click" data-placement="bottom" title="Copied URL to your clipboard"><i class="fas fa-link white"></i></a>
<a class="btn btn-default" id="copy-url-button" role="button" data-toggle="tooltip" data-trigger="click" data-placement="bottom" title="Copy URL to your clipboard"><i class="fas fa-link white"></i></a>

concordia/templates/transcriptions/asset_detail.html Outdated Show resolved Hide resolved
concordia/templates/transcriptions/asset_detail.html Outdated Show resolved Hide resolved
</div>

{% if asset.resource_url %}
<div class="btn-group btn-group-sm ml-2" role="navigation" aria-label="Link to the original item on loc.gov">
<a class="btn btn-default" target="_blank" title="View this item on loc.gov in a new tab" href="{{ asset.resource_url }}?sp={{ asset.sequence }}">View on www.loc.gov <i class="fa fa-external-link-alt"></i></a>
Copy link
Member

Choose a reason for hiding this comment

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

This title is a bit confusing since this app also lives under loc.gov. Maybe we should change the wording to something like “Open the original source for this item” or otherwise attempt to make it clear what the difference is?

@@ -28,7 +28,7 @@ <h1 class="m-3">{{ item.title }}</h1>
</div>
<div class="col-md-2 align-bottom">
<div class="m-3">
<a href="{{ item.item_url }}" class="btn btn-default" target="_blank">View this item on www.loc.gov</a>
<a href="{{ item.item_url }}" class="btn btn-default" title="View this item on loc.gov in a new tab" target="_blank">View this item on www.loc.gov <i class="fa fa-external-link-alt"></i></a>
Copy link
Member

Choose a reason for hiding this comment

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

Same question disambiguating between loc.gov, www.loc.gov, and crowd.loc.gov

@@ -679,6 +679,14 @@ def get_context_data(self, **kwargs):
.values_list("sequence", "slug")
)

ctx["current_asset_url"] = self.request.build_absolute_uri()

ctx["tweet_text"] = quote_plus(
Copy link
Member

Choose a reason for hiding this comment

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

Since this is entirely display formatting, I'm wondering whether this should be done in the page template rather than the view.

document.execCommand('copy');
$currentAssetUrl.addClass('d-none');
});
$copyUrlButton.on('shown.bs.tooltip', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the accessibility story is for auto-hiding something like this – normally I'd think that was something like adding a (possibly sr-only) element with role=alertdialog but we should ask @zuhairmahd how sensible that is for something which is basically a confirmation that the action you attempted completed without an error (see previous).

@@ -415,3 +415,23 @@ $tagEditor

displayMessage('error', message, 'tags-save-result');
});

var $copyUrlButton = $('#copy-url-button');
$copyUrlButton.tooltip();
Copy link
Member

Choose a reason for hiding this comment

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

Currently the tooltip displays on click no matter what actually happens. I'm wondering whether that should be changed to manual activation and then the JavaScript API would be used to display it after the clipboard operation is successful:

Suggested change
$copyUrlButton.tooltip();
$copyUrlButton.tooltip({trigger: "manual"});

Part of why I'm thinking about this is that while most browsers support the clipboard API past abuses have lead to a number of restrictions:

https://caniuse.com/#feat=clipboard

That makes me wonder whether the logic basically needs to be something like putting the current body of the click even handler into a try block which would display either a success message (with auto-hiding) or an error message (which doesn't go away until dismissed) after the execCommand call.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done, except for making the error message stay until dismissal... working on that part.

rstorey and others added 3 commits February 5, 2019 10:11
Co-Authored-By: rstorey <rstorey@users.noreply.github.com>
Co-Authored-By: rstorey <rstorey@users.noreply.github.com>
acdha
acdha previously approved these changes Feb 5, 2019
// Show the tooltip with a success message
tooltipMessage = 'This link has been copied to your clipboard';
$currentAssetUrl.addClass('d-none');
$(this)
Copy link
Member

Choose a reason for hiding this comment

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

Since we already have this aliased as $copyUrlButton we can avoid recreating the object:

Suggested change
$(this)
$copyUrlButton

.tooltip('dispose')
.tooltip({title: tooltipMessage})
.tooltip('show');
$(this).on('shown.bs.tooltip', hideTooltipCallback);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$(this).on('shown.bs.tooltip', hideTooltipCallback);
$copyUrlButton

Or just continue the chain for this line, too.

tooltipMessage =
'<p>Could not access your clipboard.</p><button class="btn btn-light btn-sm" id="dismiss-tooltip-button">Close</button>';
$currentAssetUrl.addClass('d-none');
$(this)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$(this)
$copyUrlButton

.tooltip({title: tooltipMessage, html: true})
.tooltip('show');
$('#dismiss-tooltip-button').on('click', function() {
$('#copy-url-button').tooltip('hide');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$('#copy-url-button').tooltip('hide');
$copyUrlButton

@rstorey rstorey changed the title WIP: Social share from the asset page Social share from the asset page Feb 5, 2019
@rstorey rstorey merged commit 1af5fc3 into master Feb 13, 2019
@rstorey rstorey deleted the social-share-asset branch March 11, 2019 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants