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

Deeplink to resource - fix #1275 #1289

Merged

Conversation

abulte
Copy link
Contributor

@abulte abulte commented Dec 4, 2017

This PR allows to make a deep link to a resource – community or standard.

A few points open to discussion:

  • maybe we do not need the "link" icon in the resource box. The URL when the modal is opened may be enough
  • there may be a cleaner solution (routes?) than hacking with location.hash – still, this seems to do the job

NB: the previous logic above https://github.com/opendatateam/udata/compare/master...abulte:gh-1275-community-resource-deeplink?expand=1#diff-50704b3aa6249195308d85bc02bc10acL70 should now be handled by .stop and .prevent modifiers on @click events.

@abulte abulte requested a review from a team December 4, 2017 16:45
Copy link
Contributor

@noirbizarre noirbizarre left a comment

Choose a reason for hiding this comment

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

Seems OK but I don't approve or reject right now, I want to know the motive behind the decision to use separate resources identifiers

@@ -64,13 +72,16 @@ new Vue({
/**
* Display a resource or a community ressource in a modal
*/
showResource(id, e, isCommunity) {
// Ensure edit button work
if ([e.target, e.target.parentNode].some(el => el.classList.contains('btn-edit'))) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this has been removed ?

location.hash = `resource${communityPrefix}-${id}`;
const modal = this.$modal(ResourceModal, {resource});
modal.$on('modal:closed', () => {
// prevent scrolling to top
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this behavior common to all modals on close ?
Then maybe be we should issue a fix in a separate PR for this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it's for this one only: since I set the location.hash with the current resource hash, I need to unset it when closing the modal. The #_ is just a trick to avoid the top scroll that would be triggered with #.

@@ -28,6 +28,9 @@ function parseUrl(url) {
return a;
}

const RESOURCE_REGEX = /^#resource-([0-9a-f-]{36})$/;
const RESOURCE_COMMUNITY_REGEX = /^#resource-community-([0-9a-f-]{36})$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think both regexp might be combined into a single one:

const RESOURCE_REGEX = /^#resource(?:-community)?-([0-9a-f-]{36})$/

Copy link
Contributor

Choose a reason for hiding this comment

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

const RESOURCE_REGEX = /^#resource(-community)?-([0-9a-f-]{36})$/ will permit to extract in a single pass the ID and the fact that's it's a community resource or not.

if (RESOURCE_REGEX.test(hash)) {
const [, id] = hash.match(RESOURCE_REGEX);
this.showResource(id, false);
} else if (RESOURCE_COMMUNITY_REGEX.test(hash)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If using a single regexp, you remove this else if

Copy link
Contributor

Choose a reason for hiding this comment

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

With the single pass regex, you can write:

const [, isCommunity, id] = hash.match(RESOURCE_REGEX);
this.showResource(id, Boolean(isCommunity));

@@ -1,12 +1,17 @@
{% set resource_format = resource.format|trim|lower or 'data' %}
<div id="resource-{{resource.id}}" class="list-group-item"
<div id="resource{% if resource.from_community %}-community{% endif %}-{{resource.id}}" class="list-group-item"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, this distinction comes from this.
As ressources IDs (including community ressources) are unique and behavior is stricly the same, is this really necessary to have distinct ID pattern ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need the distinction to use showResource that expects a isCommunity parameter, apparently in order to fetch the right resource in the JSON-LD.

The only alternative I can think of right now is to try to find the resource by id in both dataset[resources] and dataset[communityResources]. I'm not sure it's a better alternative.

@@ -28,7 +33,7 @@ <h4 class="list-group-item-heading ellipsis">
{% set edit_path = 'dataset/{id}/resource/{rid}' %}
{% endif %}
<div class="btn-group btn-group-xs tools">
<a class="btn btn-default btn-edit" v-tooltip title="{{ _('Edit') }}"
<a @click.stop class="btn btn-default btn-edit" v-tooltip title="{{ _('Edit') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG, is this working ? If so I understand the above removal !! 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it should work ;-) At least it does on my machine.

@abulte
Copy link
Contributor Author

abulte commented Dec 5, 2017

The link icon I'm talking about above:

capture d ecran 2017-12-05 10 22 28

capture d ecran 2017-12-05 10 22 36

@abulte abulte force-pushed the gh-1275-community-resource-deeplink branch from e946085 to bdb66a6 Compare December 6, 2017 16:28
@abulte abulte merged commit 8fa2882 into opendatateam:master Dec 6, 2017
@abulte abulte removed the in progress label Dec 6, 2017
@abulte abulte deleted the gh-1275-community-resource-deeplink branch December 6, 2017 16:36
@noirbizarre noirbizarre added this to the 1.2.4 milestone Dec 7, 2017
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

2 participants