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

Datahub: Improve lineage display #684

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Datahub: Improve lineage display #684

merged 2 commits into from
Nov 29, 2023

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Nov 13, 2023

The goal of this PR is to resolve #682 by making the linkify directive include trailing / in URL if present.

The line break in #682 actually comes from GN that returns Données Chambre d'agriculture des Hauts-de-France https://hautsdefrance.chambre-agriculture.fr/\nBD TOPO. 2020-11 within the lineageObject.

lineage

Copy link
Contributor

github-actions bot commented Nov 13, 2023

Affected libs: ui-elements, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, ui-catalog, ui-search,
Affected apps: metadata-editor, datahub, demo, webcomponents, search, map-viewer,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@coveralls
Copy link

coveralls commented Nov 13, 2023

Coverage Status

coverage: 85.926% (-0.03%) from 85.955%
when pulling a44eb0f on improve-lineage
into ef6a273 on main.

@fgravin fgravin added the enhancement Proposal for a new feature label Nov 15, 2023
@fgravin fgravin added this to the 2.1.0 milestone Nov 15, 2023
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks, I think the url-detecting regex needs to have stronger test to avoid any regressions. Detecting URL is a tricky topic!

I've added a list of urls to test the regex agains

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests! The E2E tests are failing on main, you can either merge like that or wait for #704 to be merged and rebase.

@tkohr
Copy link
Collaborator Author

tkohr commented Nov 28, 2023

Hum, after rebasing, I still had to re-run the e2e tests three times to pass. They randomly broke on two different tests before, that I could not reproduce locally.

Once it was datasets.cy.ts => sort by date and once home.cy.ts => only shows one record, same as the favorite one both related to the favorites feature, not sure if this is related.

@tkohr tkohr merged commit 998c2e2 into main Nov 29, 2023
8 checks passed
@tkohr tkohr deleted the improve-lineage branch November 29, 2023 09:27
@jahow
Copy link
Collaborator

jahow commented Nov 29, 2023

These tests ring a bell, we had issues with them with @cmoinier. Thanks for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datahub / Formatting of the "lineage" section is still not great
4 participants