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

SEAB-6341: Add Metrics type icons in versions tab #2048

Merged
merged 18 commits into from
Jan 13, 2025
Merged

Conversation

hyunnaye
Copy link
Contributor

@hyunnaye hyunnaye commented Dec 5, 2024

Description
This PR adds metrics type icons to indicate which type of metrics a version has.

Has both execution and validation metrics )see the version tab
image

Only one metrics type:
image

No Metrics:
image

Review Instructions
Go to various workflows and test that the icons are shown correctly and the tooltips are clear.

Workflow with only validation metrics: https://qa.dockstore.org/workflows/github.com/ICGC-TCGA-PanCancer/wdl-pcawg-sanger-cgp-workflow:master?tab=versions

Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6341

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@hyunnaye hyunnaye self-assigned this Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.83%. Comparing base (3396570) to head (bc2ceaa).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2048   +/-   ##
========================================
  Coverage    41.82%   41.83%           
========================================
  Files          395      395           
  Lines        12370    12372    +2     
  Branches      2969     2969           
========================================
+ Hits          5174     5176    +2     
  Misses        4870     4870           
  Partials      2326     2326           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

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

This is annoying feedback since I see this was the case back in the storytime mock-up, while I get "validation" from the validation icon, I'm not really getting a sense of "statistics" from the execution metrics icon.

I don't feel strongly about it but am curious how others feel and/or what the alternatives are in terms of icons

src/app/workflow/versions/versions.component.html Outdated Show resolved Hide resolved
src/app/workflow/versions/versions.component.html Outdated Show resolved Hide resolved
src/app/workflow/workflow.component.html Outdated Show resolved Hide resolved
src/app/workflow/workflow.component.html Outdated Show resolved Hide resolved
src/app/workflow/versions/versions.component.ts Outdated Show resolved Hide resolved
@@ -217,7 +217,18 @@
>
</th>
<td mat-cell *matCellDef="let version">
<mat-icon data-cy="metrics" *ngIf="version.metricsByPlatform && (version.metricsByPlatform | json) !== '{}'">check</mat-icon>
<img
*ngIf="hasExecutionMetrics"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of relying on event emitter, can't you just tell by looking a version.metricsByPlatform where there are execution and validation metrics or not? I may be missing something, and you may need a pipe to figure it out, but it seems more straightforward than having an event emitter from the executions-tab-component.

@svonworl
Copy link
Contributor

svonworl commented Dec 6, 2024

This is annoying feedback since I see this was the case back in the storytime mock-up, while I get "validation" from the validation icon, I'm not really getting a sense of "statistics" from the execution metrics icon.

Some kind of graph [bar graph?] might be good as the "executions metrics" icon, the gears typically indicate "settings"/"configuration"/etc in most UIs. Also, both current icons have a strong diagonal visual weight, which looks kinda funny, especially when they're displayed together, so one being less diagonal would be an improvement.

Copy link
Contributor

@svonworl svonworl left a comment

Choose a reason for hiding this comment

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

Don't really care for the metrics icons in the "Recent Versions" box, it's added visual clutter and feels tangential to the box's purpose. But if that's what was in the spec and the team decided was best, no need to change it.

@kathy-t
Copy link
Contributor

kathy-t commented Dec 6, 2024

This is annoying feedback since I see this was the case back in the storytime mock-up, while I get "validation" from the validation icon, I'm not really getting a sense of "statistics" from the execution metrics icon.

I don't feel strongly about it but am curious how others feel and/or what the alternatives are in terms of icons

We could re-purpose the verified icon which is just a checkmark? But then it would need to be in a bubble and might look confusing in the versions table which already has a bunch of checkmarks

image

Copy link
Contributor

@kathy-t kathy-t left a comment

Choose a reason for hiding this comment

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

Was here, following discussion.

The Recent Versions box does look busy when both icons are there and the box is kind of small. Maybe it's not worth it to have it there, or we just display a generic metrics icon, but then there would be 3 metrics icons and that might be too much

@denis-yuen
Copy link
Member

Don't really care for the metrics icons in the "Recent Versions" box, it's added visual clutter and feels tangential to the box's purpose. But if that's what was in the spec and the team decided was best, no need to change it.

On the fence on this one, I agree that the icons in the more detailed versions tab is more intuitive

@hyunnaye
Copy link
Contributor Author

hyunnaye commented Dec 12, 2024

Updated screenshots to show the new execution metrics icon and removed the icons on the recent versions sidebar. Need to wait until dockstore/dockstore#6055 merge

@kathy-t
Copy link
Contributor

kathy-t commented Dec 12, 2024

Updated screenshots to show the new execution metrics icon and removed the icons on the recent versions sidebar. Need to wait until dockstore/dockstore#6055 merge

FYI, you can update the webservice version in the package.json so it builds against your webservice branch if you want to see if the UI builds pass before the webservice PR is merged.

You would modify it like https://github.com/dockstore/dockstore-ui2/pull/2046/files, except your webservice_version_prefix would be 1.17.0-feature-seab-6341

@denis-yuen denis-yuen changed the title SEAB-6341: Add Metrics type icons in versions tab and Rescent Versions sidebar SEAB-6341: Add Metrics type icons in versions tab and Recent Versions sidebar Dec 12, 2024
@hyunnaye hyunnaye changed the title SEAB-6341: Add Metrics type icons in versions tab and Recent Versions sidebar SEAB-6341: Add Metrics type icons in versions tab Dec 12, 2024
src/app/workflow/versions/versions.component.html Outdated Show resolved Hide resolved
src/app/workflow/versions/versions.component.html Outdated Show resolved Hide resolved
src/app/workflow/versions/versions.component.ts Outdated Show resolved Hide resolved
src/app/workflow/versions/versions.component.ts Outdated Show resolved Hide resolved
Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

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

Ok with current set of icons, after addressing feedback from others

Copy link
Contributor

@kathy-t kathy-t left a comment

Choose a reason for hiding this comment

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

Design looks good. Would second Charles' feedback about the pipe

showExecutionMetricsIcon(version: WorkflowVersion) {
if (version.metricsByPlatform !== null) {
const metrics = version.metricsByPlatform[PartnerEnum.ALL];
return metrics?.executionStatusCount != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

!= -> !==

showValidationMetricsIcon(version: WorkflowVersion) {
if (version.metricsByPlatform !== null) {
const metrics = version.metricsByPlatform[PartnerEnum.ALL];
return metrics?.validationStatus != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@hyunnaye hyunnaye requested a review from kathy-t January 13, 2025 20:46
@hyunnaye hyunnaye merged commit 9792e69 into develop Jan 13, 2025
18 of 20 checks passed
@hyunnaye hyunnaye deleted the feature/SEAB-6341 branch January 13, 2025 21:52
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.

5 participants