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

Apply links to browse taxon -page [closes #696] #755

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

rpulkka
Copy link
Contributor

@rpulkka rpulkka commented Feb 28, 2025

https://696.dev.laji.fi/taxon/browse?informalGroupFilters=MVL.343
#696

Three changes:

  1. Made the "informal-list-breadcrumb" links (the path Kaikki / Putkilokasvit... etc.) clickable with right mouse button so that they can be opened to a new tab. This can be done by adding [routerLink], and taxon id as [queryParams].
  2. Made the "informal-list" buttons clickable with the same method.
  3. Used cell-template 'taxonScientificNameLink' for the list's scientificName column and changed the cell-template to check whether row.id is a link or id, and form the [routerLink] accordingly. It should therefore work with taxa and subordinate taxa.

@rpulkka rpulkka changed the title Apply links to browse taxon -page. Apply links to browse taxon -page [closes #696] Feb 28, 2025
@Blodir Blodir self-assigned this Mar 3, 2025
@@ -145,7 +145,8 @@
</span>
</ng-template>
<ng-template let-row="row" #taxonScientificNameLink>
<a *ngIf="row.scientificName" title="{{row.scientificName}}" [routerLink]="['/taxon/' + row.id] | localize">
<a *ngIf="row.scientificName" title="{{row.scientificName}}"
[routerLink]="['/taxon/' + (row.id.includes('http') ? row.id.split('/').pop() : row.id)] | localize">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is highly suspicious. Why is the id property an url? Sounds like the data needs to be parsed earlier to parse the id out of the url...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I found a code block from species-list.component.ts now which seems to transform ID to url for some reason. I deleted it now, and it doesn't return url from ID anymore.

@@ -1,9 +1,11 @@
<ol class="breadcrumb" *ngIf="informalGroup && informalGroup.id">
<li routerLinkActive="active" [routerLinkActiveOptions]="{exact: true}">
<a (click)="informalGroupSelect.emit()" tabindex="0" role="button" luKeyboardClickable>{{ 'observation.active.informalTaxonGroupAll' | translate }}</a>
<a [routerLink]="'/taxon/browse'" (click)="informalGroupSelect.emit()"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use click events on a elements, this creates a collision with default browser behavior (you can actually visually see some weirdness if you compare how the informal group select behaves on dev.laji.fi vs this branch). The component needs to be refactored to remove the informalGroupSelect event entirely from both informal-list and informal-list-breadcrumb. Instead, add a queryparams change listener, and use that to update the "search query"

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 tried to subscribe to query params now, is it the type of change listener that you meant?

@rpulkka rpulkka requested a review from Blodir March 3, 2025 12:54
Copy link
Contributor

@Blodir Blodir 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!

@Blodir Blodir added this to the Release 80 milestone Mar 3, 2025
@Blodir Blodir merged commit b86bd27 into development Mar 3, 2025
1 check passed
@Blodir Blodir deleted the taxon-page-links-696 branch March 3, 2025 13:30
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