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

Add ability to link locations with the wider document automatically #517

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

piemonkey
Copy link
Collaborator

@piemonkey piemonkey commented Dec 9, 2024

Overview

Also move from dct:spatial to prov:atLocation.

I spent some time refactoring the address variable to re-use a load of the code from the oslo location node and so to benefit from these changes, but I realised that substantially increases the scope of the changes. Instead of finishing those changes, I've not committed them. If we think we should include them, I'll add them, otherwise I'll hold on to them for now.

The links are made in RDFa but only the backlink works in the RDFa tools. This is unfixed as we need to refactor how these nodes work anyway, so should do this then.

Edit: In order to avoid a weird situation of having 2 prov:atLocation triples with different objects for each location, I included a more substantial refactor which 'de-variable-ifies' the location code. So, now:

  • It no longer inserts a placeholder first, then edits that placeholder with the address. We perhaps need an 'insert placeholder' for building templates, but the old behaviour was weird anyway, as it involved inserting a location, then closing the modal without setting it, which is not ideal.
  • It no longer inserts a load of extra RDFa for 'mappings' and variable types.
  • Supports importing locations from before this change, but does not remove the 'extra' RDFa until the location is edited.

Things this does not include:

  • A fix for the 'incorrect RDFa' issue, which is a quick fix, but needs more work to also support address variables.
  • Support for template UUIDs in locations. It's unclear whether we want this as we no longer have the 'mappings', but it would be a simple fix to add if we want it.
connected issues and PRs:

Jira ticket: https://binnenland.atlassian.net/browse/GN-5022
GN PR: lblod/frontend-gelinkt-notuleren#794
RB PR: lblod/frontend-reglementaire-bijlage#306

Setup

N/A

How to test/reproduce

In the besluit or snippet sample pages, insert locations into arbitrary documents, inside Besluits and inside Articles. The expectation (see jira) is that it will link the location to an article if it can find one or a besluit if it cannot. If it can't find either, it will be unlinked.

Challenges/uncertainties

N/A

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check if dummy app is correctly updated
  • Check cancel/go-back flows
  • changelog
  • npm lint
  • no new deprecations

Copy link
Collaborator

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Looks good to me!

  • When inserting a location in a decision/article, it's correctly linked
  • The RDFa seems more correct/less verbose now. And seems to be following the data-model correctly.

I do think we'll need a template-version down the road.

@piemonkey piemonkey merged commit c16f0b3 into master Dec 16, 2024
3 checks passed
@piemonkey piemonkey deleted the feat/link-locations branch December 16, 2024 09:40
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.

2 participants