feat: templated external links #1910
Conversation
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[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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>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
Show resolved
Hide resolved
src/app/publisheddata/publisheddata-details/publisheddata-details.component.html
Show resolved
Hide resolved
|
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: Line 213 in 5f56da2 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.)
nitrosx
left a comment
nitrosx
left a comment
There was a problem hiding this comment.
Once the changes requested are addressed, I will be happy to approve it
| // Accessory types and definition for Angular component TableComponent | ||
|
|
There was a problem hiding this comment.
Are we changing the colors in the vanilla version of SciCat?
There was a problem hiding this comment.
Yes, this change affects the color of "header-3" in the standard version. ( See explanation here #1910 (comment) )
There was a problem hiding this comment.
Same as above, are we changing the color of the vanilla version of SciCat?
src/app/datasets/dataset-detail/dataset-detail/dataset-detail.component.html
Outdated
Show resolved
Hide resolved
|
@GBirkel is this a draft PR or is it ready to be reviewed? |
Ah, yes, this is a draft. Sorry for the confusion. |
…ted-external-links
…h links directly after fetching dataset when preparing the dataset view component.
Merge branch 'refs/heads/master' into templated-external-links
…during development of this PR). These changes are now in a separate PR.
GBirkel
changed the title
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the new Links card, use
target="_blank"instead oftarget="blank"and consider addingrel="noopener noreferrer"on the anchor to avoid security and performance issues when opening external links. - In
DatasetEffects, the combined loading effect now listens forfetchExternalLinksCompleteAction/FailedActioninstead offetchOrigDatablocksCompleteAction/FailedAction, which looks like a copy‑paste mistake and may break the loading state for origDatablocks. - Consider resetting
currentSetExternalLinkswhen a new dataset is selected orfetchDatasetActionis dispatched so that links from a previously viewed dataset are not briefly shown for the next dataset before its links load.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new Links card, use `target="_blank"` instead of `target="blank"` and consider adding `rel="noopener noreferrer"` on the anchor to avoid security and performance issues when opening external links.
- In `DatasetEffects`, the combined loading effect now listens for `fetchExternalLinksCompleteAction`/`FailedAction` instead of `fetchOrigDatablocksCompleteAction`/`FailedAction`, which looks like a copy‑paste mistake and may break the loading state for origDatablocks.
- Consider resetting `currentSetExternalLinks` when a new dataset is selected or `fetchDatasetAction` is dispatched so that links from a previously viewed dataset are not briefly shown for the next dataset before its links load.
## Individual Comments
### Comment 1
<location> `src/app/datasets/dataset-detail/dataset-detail/dataset-detail.component.html:364` </location>
<code_context>
+ <mat-card *ngIf="externalLinks$ | async as externalLinks">
+ <mat-card-header class="dataset-links-header">
+ <mat-icon class="section-icon"> link </mat-icon>
+ <mat-card-title class="section-title">{{ "Links" | translate }}</mat-card-title>
+ </mat-card-header>
+ <mat-card-content>
</code_context>
<issue_to_address>
**question (bug_risk):** Translation usage for the new "Links" title is inconsistent with the rest of the template
Other headers in this template pass the `localization` parameter to `translate` (e.g. `"General Information" | translate: localization`), but this one does not. If `localization` is required for correct lookup, this title may be translated inconsistently with the other sections.
</issue_to_address>
### Comment 2
<location> `src/app/datasets/dataset-detail/dataset-detail/dataset-detail.component.html:371` </location>
<code_context>
+ <tr *ngFor="let externalLink of externalLinks">
+ <th>{{ externalLink.title }}</th>
+ <td>
+ <a href="{{ externalLink.url }}" target="blank"
+ >{{ externalLink.description }}</a
+ >
</code_context>
<issue_to_address>
**🚨 issue (security):** External link anchor should use proper target value and security attributes
Consider updating this to `target="_blank"` and adding `rel="noopener noreferrer"` so external links opened in a new tab don’t have access to `window.opener`. You can also use property binding for the URL: `[href]="externalLink.url"`.
</issue_to_address>
### Comment 3
<location> `src/app/state-management/reducers/datasets.reducer.ts:67-70` </location>
<code_context>
},
),
+ on(
+ fromActions.fetchExternalLinksCompleteAction,
+ (state, { externallinks }) => {
+ return {
+ ...state,
+ currentSetExternalLinks: externallinks,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** External links state is only updated on success and may show stale data when switching datasets
Since `currentSetExternalLinks` is only updated on `fetchExternalLinksCompleteAction`, switching datasets quickly will briefly show links from the previous dataset until the new fetch completes. Consider also handling `fetchDatasetAction`/`fetchExternalLinksAction` to reset `currentSetExternalLinks` (e.g., to an empty array) when a new dataset starts loading, so outdated links aren’t shown.
</issue_to_address>
### Comment 4
<location> `src/app/state-management/effects/datasets.effects.ts:449-450` </location>
<code_context>
fromActions.fetchDatablocksFailedAction,
- fromActions.fetchOrigDatablocksCompleteAction,
- fromActions.fetchOrigDatablocksFailedAction,
+ fromActions.fetchExternalLinksCompleteAction,
+ fromActions.fetchExternalLinksFailedAction,
fromActions.fetchAttachmentsCompleteAction,
fromActions.fetchAttachmentsFailedAction,
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing OrigDatablocks complete/failed actions from this list may prevent loading state from being cleared
These actions previously included `fetchOrigDatablocksCompleteAction` / `fetchOrigDatablocksFailedAction`, which are now replaced by the external links actions. If this effect is responsible for clearing loading flags or other post-fetch cleanup, OrigDatablocks requests may no longer signal completion/failure here, leaving the UI stuck loading or skipping cleanup. Unless this change is handled elsewhere, keep the OrigDatablocks actions and add the external links actions instead of replacing them.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <mat-card *ngIf="externalLinks$ | async as externalLinks"> | ||
| <mat-card-header class="dataset-links-header"> | ||
| <mat-icon class="section-icon"> link </mat-icon> | ||
| <mat-card-title class="section-title">{{ "Links" | translate }}</mat-card-title> |
There was a problem hiding this comment.
question (bug_risk): Translation usage for the new "Links" title is inconsistent with the rest of the template
Other headers in this template pass the localization parameter to translate (e.g. "General Information" | translate: localization), but this one does not. If localization is required for correct lookup, this title may be translated inconsistently with the other sections.
| on( | ||
| fromActions.fetchExternalLinksCompleteAction, | ||
| (state, { externallinks }) => { | ||
| return { |
There was a problem hiding this comment.
suggestion (bug_risk): External links state is only updated on success and may show stale data when switching datasets
Since currentSetExternalLinks is only updated on fetchExternalLinksCompleteAction, switching datasets quickly will briefly show links from the previous dataset until the new fetch completes. Consider also handling fetchDatasetAction/fetchExternalLinksAction to reset currentSetExternalLinks (e.g., to an empty array) when a new dataset starts loading, so outdated links aren’t shown.
| fromActions.fetchExternalLinksCompleteAction, | ||
| fromActions.fetchExternalLinksFailedAction, |
There was a problem hiding this comment.
issue (bug_risk): Removing OrigDatablocks complete/failed actions from this list may prevent loading state from being cleared
These actions previously included fetchOrigDatablocksCompleteAction / fetchOrigDatablocksFailedAction, which are now replaced by the external links actions. If this effect is responsible for clearing loading flags or other post-fetch cleanup, OrigDatablocks requests may no longer signal completion/failure here, leaving the UI stuck loading or skipping cleanup. Unless this change is handled elsewhere, keep the OrigDatablocks actions and add the external links actions instead of replacing them.
3 participants
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#2194 .
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
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
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
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:
Enhancements:
Summary by Sourcery
Add support for displaying admin-configured external links on the dataset detail view and align header styling and theming across dataset, published data, and reduce views.
New Features:
Enhancements: