feat: templated external links (draft) #1910

GBirkel · 2025-06-24T22:01:51Z

Description

Add a field for generating dataset links based on admin-configured templates.

See SciCatProject/scicat-backend-next#689 for details.

This has a companion PR for the back end: SciCatProject/scicat-backend-next#2097 .

Motivation

We get a lot of requests from users and researchers to provide a variety of different types of web-based tools that related directly to datasets. These can be pages that display visualization, pages that provide analysis tools (including AI/ML) and jupyter notebooks on particular JupyterHub instance. In all cases, we would want to take the user to the page with the dataset in context.

Fixes:

Please provide a list of the fixes implemented in this PR

Items added

Changes:

Please provide a list of the changes implemented by this PR

changes made

Tests included

Included for each change/fix?
Passing? (Merge will not be approved unless this is checked)

Documentation

swagger documentation updated [required]
official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

Does it require a specific version of the backend
which version of the backend is required:

Summary by Sourcery

Add a new “Links” section to dataset detail and unify header styling across various components by renaming CSS classes and updating theme color mappings

New Features:

Add a “Links” card to the dataset detail view listing proposal, sample, instrument, creation location, techniques, and input datasets with clickable links

Enhancements:

Rename header classes for dataset-detail, publisheddata-details, and reduce components to a consistent naming scheme
Update SCSS theme files and global styles to reflect renamed header classes and correct header-3 color assignments in theme.ts

Catching up to main repo

Catching up to home base

… of color assignment for headers in dataset details.

…of the other header colors, keeping the same hue.

… my walk through the rest of the codebase.

sourcery-ai

Hey @GBirkel - I've reviewed your changes - here's some feedback:

I noticed the SCSS class names for published data headers are spelled “pubishedata-” instead of “publisheddata-” – please fix these typos to avoid broken styles.
The new Links table in the template has a lot of repetitive blocks; consider refactoring these into a small reusable component or using an *ngFor over a link-config array to make the template more maintainable.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- I noticed the SCSS class names for published data headers are spelled “pubishedata-” instead of “publisheddata-” – please fix these typos to avoid broken styles.
- The new Links table in the template has a lot of repetitive <tr> blocks; consider refactoring these into a small reusable component or using an *ngFor over a link-config array to make the template more maintainable.

## Individual Comments

### Comment 1
<location> `src/app/publisheddata/publisheddata-details/_publisheddata-details-theme.scss:12` </location>
<code_context>
   $accent: map.get($color-config, "accent");
   $header-2: map.get($color-config, "header-2");
   mat-card {
-    .status-header {
+    .pubishedata-status-header {
       background-color: mat.m2-get-color-from-palette($warn, "lighter");
     }

-    .general-header {
+    .pubishedata-general-header {
       background-color: mat.m2-get-color-from-palette($primary, "lighter");
     }

-    .creator-header {
+    .pubishedata-creator-header {
       background-color: mat.m2-get-color-from-palette($header-1, "lighter");
     }

-    .file-header {
-      background-color: mat.m2-get-color-from-palette($accent, "lighter");
+    .pubishedata-file-header {
+      background-color: mat.m2-get-color-from-palette($header-2, "lighter");
     }

-    .related-header {
-      background-color: mat.m2-get-color-from-palette($header-2, "lighter");
+    .pubishedata-related-header {
</code_context>

<issue_to_address>
Typo in class name: 'pubishedata' should be 'publisheddata'.

Using 'pubishedata' instead of 'publisheddata' may cause confusion and inconsistency. Please update the class names to 'publisheddata-*' for clarity.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    .pubishedata-status-header {
      background-color: mat.m2-get-color-from-palette($warn, "lighter");
    }

    .pubishedata-general-header {
      background-color: mat.m2-get-color-from-palette($primary, "lighter");
    }

    .pubishedata-creator-header {
      background-color: mat.m2-get-color-from-palette($header-1, "lighter");
    }

    .pubishedata-file-header {
      background-color: mat.m2-get-color-from-palette($header-2, "lighter");
    }

    .pubishedata-related-header {
      background-color: mat.m2-get-color-from-palette($accent, "lighter");
    }
=======
    .publisheddata-status-header {
      background-color: mat.m2-get-color-from-palette($warn, "lighter");
    }

    .publisheddata-general-header {
      background-color: mat.m2-get-color-from-palette($primary, "lighter");
    }

    .publisheddata-creator-header {
      background-color: mat.m2-get-color-from-palette($header-1, "lighter");
    }

    .publisheddata-file-header {
      background-color: mat.m2-get-color-from-palette($header-2, "lighter");
    }

    .publisheddata-related-header {
      background-color: mat.m2-get-color-from-palette($accent, "lighter");
    }
>>>>>>> REPLACE

</suggested_fix>

### Comment 2
<location> `src/app/publisheddata/publisheddata-details/publisheddata-details.component.html:4` </location>
<code_context>
 <div fxLayout="row" fxLayout.xs="column" *ngIf="publishedData">
   <div fxFlex="60">
     <mat-card>
-      <mat-card-header class="status-header">
+      <mat-card-header class="pubishedata-status-header">
         <div mat-card-avatar class="section-icon">
</code_context>

<issue_to_address>
Class name typo: 'pubishedata' should likely be 'publisheddata'.

Using the correct spelling will improve consistency and make it easier to locate related styles.

Suggested implementation:

```
      <mat-card-header class="publisheddata-status-header">

```

```
      <mat-card-header class="publisheddata-general-header">

```
</issue_to_address>

### Comment 3
<location> `src/styles.scss:213` </location>
<code_context>
     warn-2: $warn-2,
     header-1: $header-1,
     header-2: $header-2,
-    header-3: $header-4,
+    header-3: $header-3,
     header-4: $header-4,
</code_context>

<issue_to_address>
Potential previous bug: header-3 was mapped to header-4.

This change should correct any color inconsistencies. Please verify all usages to confirm the issue is fully resolved.

Suggested implementation:

```
    header-3: $header-3,

```

You should also:
1. Search for all usages of the `header-3` color variable in your codebase (e.g., `color: map-get($theme-colors, header-3)` or similar).
2. Verify that these usages now display the correct color as defined by `$header-3`.
3. If you find any components or styles that were relying on the previous (incorrect) mapping to `$header-4`, update them to use the correct color variable as intended.

No further changes are needed in `src/styles.scss` itself, but you may need to update other files if you find usages that were depending on the old mapping.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/app/publisheddata/publisheddata-details/_publisheddata-details-theme.scss

src/app/publisheddata/publisheddata-details/publisheddata-details.component.html

src/styles.scss

Junjiequan · 2025-06-27T08:20:32Z

Hi I see you have changed some theme colors, could you provide screenshots and explain why the theme color change is needed

…d-external-links

GBirkel · 2025-07-17T21:30:40Z

Hi I see you have changed some theme colors, could you provide screenshots and explain why the theme color change is needed

Certainly!

Currently, SciCat's styling is configured to define four header styles, 1 through 4, except style 3 and 4 are mapped to the same thing:

frontend/src/styles.scss

Line 213 in 5f56da2

header-3: $header-4,

This change was introduced about four years ago during what appears to be an unrelated refactor:

0e4720b#diff-23fad3928bf3e71585eb4a6e3b2ff72dae02c3ed5a44d2f0deb1d49f0cb3b1a3R207

If anyone has insight into why, I'd be very interested to know. Right now it just looks like a copy-paste error.

Anyway, as a result of that, for four years we've had two header subsections with the same color. Check out "File Information" and "Derived Data":

When I changed header 3 to actually point to header style 3, the color that appeared had a luminance value that was quite different from the other header colors. It was much brighter. I also noticed that the current ordering of the header colors went across the spectrum out-of-order, instead of consistently from one end of the spectrum to the other. So I dialed down the luminance of the newly revealed color and changed the order:

We still don't have enough colors to make every subsection different, but there's one more to work with at least. This is important because I'm adding another section: "Links", visible (with junk content) in the above screenshot.

…h API spec directly from within Docker due to "backend.localhost" mapping.)

…atLive.

nitrosx

Once the changes requested are addressed, I will be happy to approve it

nitrosx · 2025-07-29T08:48:10Z

src/app/shared/modules/table/table.component.spec.ts

+// Jasmine testing framework descriptions for Angular component TableComponent
+


Do we really need this comment here?

nitrosx · 2025-07-29T08:48:25Z

src/app/shared/modules/table/table.component.ts

+// Accessory types and definition for Angular component TableComponent
+


Do we need the comment?

nitrosx · 2025-07-29T08:48:35Z

src/app/shared/modules/table/table.module.ts

+// NgModule declaration for Angular component TableComponent
+


Do we need the comment?

nitrosx · 2025-07-29T08:49:28Z

src/app/theme.ts

Are we changing the colors in the vanilla version of SciCat?

nitrosx · 2025-07-29T08:49:54Z

src/assets/theme.json

Same as above, are we changing the color of the vanilla version of SciCat?

nitrosx · 2025-07-29T08:50:23Z

src/app/shared/modules/table/table.component.html

Can we remove the comments?

nitrosx · 2025-07-29T08:51:51Z

src/app/datasets/dataset-detail/dataset-detail/dataset-detail.component.html

+          <table>
+            <tr *ngIf="dataset['proposalId'] && proposal">
+              <th>{{ "Proposal" | translate }}</th>
+              <td>
+                <a (click)="onClickProposal(proposal.proposalId)">{{
+                  proposal.title
+                }}</a>
+              </td>
+            </tr>
+            <tr
+              *ngIf="
+                dataset['proposalId'] &&
+                !proposal &&
+                appConfig.datasetDetailsShowMissingProposalId
+              "
+            >
+              <th>{{ "Proposal Id" | translate }}</th>
+              <td>{{ dataset["proposalId"] }}</td>
+            </tr>
+            <tr *ngIf="dataset['sampleId'] && sample">
+              <th>{{ "Sample" | translate }}</th>
+              <td>
+                <a (click)="onClickSample(sample.sampleId)">
+                  <span>{{ sample.description }}</span>
+                </a>
+              </td>
+            </tr>
+            <tr *ngIf="dataset['instrumentId'] && instrument">
+              <th>{{ "Instrument" | translate }}</th>
+              <td>
+                <a (click)="onClickInstrument(instrument.pid)">
+                  {{ instrument.name }}
+                </a>
+              </td>
+            </tr>
+            <tr *ngIf="dataset['creationLocation'] as value">
+              <th>{{ "Creation Location" | translate }}</th>
+              <td>{{ value }}</td>
+            </tr>
+            <tr *ngIf="dataset.techniques && dataset.techniques.length > 0">
+              <th>{{ "Techniques" | translate }}</th>
+              <td>
+                <div *ngFor="let technique of dataset.techniques">
+                  <span>
+                    {{ technique.name }}
+                  </span>
+                  <span
+                    *ngIf="
+                      dataset.techniques.indexOf(technique) <
+                      dataset.techniques.length - 1
+                    "
+                    >,
+                  </span>
+                </div>
+              </td>
+            </tr>


I'm not sure I understand why we added these fields here as they should be already present in other section of the dataset detail template.

nitrosx · 2025-08-08T09:26:31Z

scripts/generate-nestjs-sdk.bash

We should make this code more robust.
Please read my comments below and see if they make sense.

nitrosx · 2025-08-08T09:32:51Z

scripts/generate-nestjs-sdk.bash

+USING_SCICAT_LIVE=0
+while [[ "$#" -gt 0 ]]; do
+	case "$1" in
+		--scicatlive)
+			USING_SCICAT_LIVE=1
+			shift
+			;;
+		*)
+			echo "Unknown argument: $1"
+			exit 1
+			;;
+	esac
+done
+


I would change this section of the code like below. This way we have a default, but we do not hardcode the all the other possible values, but we provide the user the option to specify the correct URL, including the case where the backend is running on a different port or different url.

SWAGGER_API_URL="http://localhost:3000/explorer-json" while [[ "$#" -gt 0 ]]; do case "$1" in --swagger-url) shift SWAGGER_API_URL=$1 shift ;; *) echo "Unknown argument: $1" exit 1 ;; esac done

nitrosx · 2025-08-08T09:33:53Z

scripts/generate-nestjs-sdk.bash

+if [ $USING_SCICAT_LIVE -eq 1 ];
+then
+	# For when developing with SciCat Live:
+	curl http://backend.localhost/explorer-json > local-api-for-generator.json
+else
+	# For when developing with a backend running directly on localhost:
+	curl http://host.docker.internal:3000/explorer-json > local-api-for-generator.json
+fi


change the code to:

curl ${SWAGGER_API_URL} > local-api-for-generator.json

nitrosx · 2025-08-08T11:31:53Z

@GBirkel is this a draft PR or is it ready to be reviewed?
If it is a draft, as it is mentioned in the title, can we change the status to draft, pretty please?

GBirkel · 2025-08-08T18:14:51Z

@GBirkel is this a draft PR or is it ready to be reviewed? If it is a draft, as it is mentioned in the title, can we change the status to draft, pretty please?

Ah, yes, this is a draft. Sorry for the confusion.

GBirkel and others added 7 commits June 3, 2025 12:59

Merge pull request #3 from SciCatProject/master

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a5a3c28

Catching up to main repo

Merge pull request #4 from SciCatProject/master

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

9bc67e1

Catching up to home base

Dealing with CSS name collisions in browser space. Slight re-ordering…

0e97ce2

… of color assignment for headers in dataset details.

Adding a header style for links.

004482b

Draft dataset links card.

7255794

Changing this color to one that matches the saturation and luminance …

237d4a1

…of the other header colors, keeping the same hue.

Seems to be a typo. Can't find any reason why this would be here from…

f263e26

… my walk through the rest of the codebase.

sourcery-ai bot reviewed Jun 24, 2025

View reviewed changes

src/app/publisheddata/publisheddata-details/_publisheddata-details-theme.scss Outdated Show resolved Hide resolved

src/app/publisheddata/publisheddata-details/publisheddata-details.component.html Show resolved Hide resolved

src/styles.scss Show resolved Hide resolved

GBirkel and others added 3 commits July 15, 2025 15:57

Oops; fixing a typo.

946f145

Merge commit '5f56da2d2a8f1b981ecbe1fa875971af64a58194' into template…

67671bb

…d-external-links

Adapting the newly introduced style class names.

0f7c8e8

GBirkel added 4 commits July 22, 2025 14:54

Patching the local SDK generator to support SciCat live. (Cannot fetc…

3cbeb67

…h API spec directly from within Docker due to "backend.localhost" mapping.)

Minor comment to patched script.

16c4dec

Development SDK generation script patches, for playing nice with SciC…

584613f

…atLive.

Just some minor code commentary.

6b407e3

GBirkel mentioned this pull request Aug 4, 2025

feat: templated external links (draft) SciCatProject/scicat-backend-next#2097

Draft

4 tasks

Merge branch 'refs/heads/master' into templated-external-links

12c11b8

Junjiequan requested a review from nitrosx August 6, 2025 11:58

nitrosx requested changes Aug 8, 2025

View reviewed changes

GBirkel marked this pull request as draft August 8, 2025 18:10

Accepting an alternate Swagger URL via command line argument.

e9b65e3

		// Jasmine testing framework descriptions for Angular component TableComponent

		// Accessory types and definition for Angular component TableComponent

		// NgModule declaration for Angular component TableComponent

feat: templated external links (draft) #1910

Are you sure you want to change the base?

feat: templated external links (draft) #1910

Conversation

GBirkel commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Fixes:

Changes:

Tests included

Documentation

official documentation info

Backend version

Summary by Sourcery

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Junjiequan commented Jun 27, 2025

Uh oh!

GBirkel commented Jul 17, 2025

Uh oh!

nitrosx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitrosx commented Aug 8, 2025

Uh oh!

GBirkel commented Aug 8, 2025

Uh oh!

GBirkel commented Jun 24, 2025 •

edited

Loading