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.

Sign up for GitHub

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

Jump to bottom

DOCK-1689: added primary indicator and icon #1664

Merged
merged 34 commits into from
Jan 9, 2023
Merged

DOCK-1689: added primary indicator and icon #1664

merged 34 commits into from
Jan 9, 2023

Conversation

hyunnaye
Copy link
Contributor

@hyunnaye hyunnaye commented Dec 20, 2022 •
edited
Loading

Description
This PR adds a primary indicator to the primary descriptor file of workflows. It also adds an icon to navigate to the primary descriptor when viewing non-primary files.

When viewing primary descriptor
Screenshot from 2022-12-20 11-23-06
Screenshot from 2022-12-20 11-23-24

Screenshot from 2022-12-20 11-27-36
Screenshot from 2022-12-20 11-27-14

When viewing non-primary file
Screenshot from 2022-12-19 14-37-47
Screenshot from 2022-12-19 14-37-34

Review Instructions
Verify the primary descriptor is correctly identified and the home icon navigates to the primary descriptor.

Issue
dockstore/dockstore#5044
dockstore/dockstore#4004

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.

@codecov
Copy link

codecov bot commented Jan 3, 2023 •
edited
Loading

Codecov Report

Base: 43.09% // Head: 40.71% // Decreases project coverage by -2.38% ⚠️

Coverage data is based on head (73c408f) compared to base (e9c5621).
Patch coverage: 42.85% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1664      +/-   ##
===========================================
- Coverage    43.09%   40.71%   -2.39%     
===========================================
  Files          314      348      +34     
  Lines         9702    10589     +887     
  Branches      2470     2717     +247     
===========================================
+ Hits          4181     4311     +130     
- Misses        3469     4048     +579     
- Partials      2052     2230     +178
Impacted Files Coverage Δ
src/app/shared/modules/workflow.module.ts 42.85% <ø> (ø)
...app/source-file-tabs/source-file-tabs.component.ts 25.92% <22.22%> (-1.74%) ⬇️
src/app/file-tree/file-tree.component.ts 52.17% <80.00%> (-0.21%) ⬇️
...rc/app/home-page/home-logged-out/home.component.ts 65.78% <0.00%> (-2.64%) ⬇️
.../home-page/widget/entry-box/entry-box.component.ts 35.71% <0.00%> (-1.79%) ⬇️
src/app/search/state/search.service.ts 23.25% <0.00%> (ø)
...ernal/change-username/change-username.component.ts 47.72% <0.00%> (ø)
src/app/shared/modules/snackbar.module.ts 42.85% <0.00%> (ø)
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

denis-yuen
denis-yuen reviewed Jan 5, 2023
View reviewed changes
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.

build issue, monitoring above comments

kathy-t
kathy-t reviewed Jan 5, 2023
View reviewed changes
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.

Noticed some things when running it locally:

The dropdown shrunk (probably a side effect of the new changes):
image

The Test Parameter Files and Configuration tabs have the "Go to primary descriptor" button. I don't think they should:
image
image

src/app/source-file-tabs/source-file-tabs.component.html Outdated Show resolved Hide resolved
src/app/source-file-tabs/source-file-tabs.service.ts Outdated Show resolved Hide resolved
src/app/source-file-tabs/source-file-tabs.service.ts Outdated Show resolved Hide resolved
@hyunnaye
Copy link
Contributor Author

hyunnaye commented Jan 6, 2023

Noticed some things when running it locally:

The dropdown shrunk (probably a side effect of the new changes): image

The Test Parameter Files and Configuration tabs have the "Go to primary descriptor" button. I don't think they should: image image

dropdown sizing error now fixed in new commit

@hyunnaye hyunnaye requested a review from kathy-t January 6, 2023 17:42
coverbeck
coverbeck reviewed Jan 6, 2023
View reviewed changes
Copy link
Collaborator

@coverbeck coverbeck 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! I think there are a couple of remnants left over from your latest refactoring that I flagged, but otherwise it's ready. Thanks for dealing with all my comments.

src/app/source-file-tabs/source-file-tabs.component.ts Outdated Show resolved Hide resolved
src/app/test/service-stubs.ts Outdated Show resolved Hide resolved
src/app/app.module.ts Outdated Show resolved Hide resolved
src/app/file-tree/file-tree.component.ts Outdated Show resolved Hide resolved
@hyunnaye hyunnaye requested a review from coverbeck January 6, 2023 18:46
kathy-t
kathy-t reviewed Jan 6, 2023
View reviewed changes
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.

Thanks for addressing the dropdown sizing!

I think you may have missed my second comment about the "Go to primary descriptor" button on the non Descriptor Files tab (below). This is the workflow I'm seeing the button on http://localhost:4200/workflows/github.com/iwc-workflows/sars-cov-2-pe-illumina-wgs-variant-calling/COVID-19-PE-WGS-ILLUMINA:main?tab=files

The Test Parameter Files and Configuration tabs have the "Go to primary descriptor" button. I don't think they should: image image

denis-yuen
denis-yuen approved these changes Jan 9, 2023
View reviewed changes
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.

After addressing Kathy's comment, otherwise, looks good!

@hyunnaye hyunnaye requested a review from kathy-t January 9, 2023 18:49
kathy-t
kathy-t reviewed Jan 9, 2023
View reviewed changes
src/app/source-file-tabs/source-file-tabs.component.html Outdated
@@ -38,7 +38,7 @@
</button>
</span>
<span fxFlex="40">
<span *ngIf="fileTab.value.length > 10" fxLayoutAlign="center center">
<span *ngIf="fileTab.value.length > 10 && fileTab.key === 'Descriptor Files'" fxLayoutAlign="center center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think fileTab.key === 'Descriptor Files' should be with your go to primary button on line 53 instead (like how you did it on line 86). Hypothetically, the Test Parameter Files tab could have more than 10 files in which case this change would not display the file name for that tab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@hyunnaye hyunnaye requested a review from kathy-t January 9, 2023 20:24
@hyunnaye hyunnaye merged commit 69e90ad into develop Jan 9, 2023
@hyunnaye hyunnaye deleted the feature/DOCK-1689/primary-descriptor-indicator branch January 9, 2023 20:53
@unito-bot unito-bot mentioned this pull request Jan 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svonworl svonworl svonworl approved these changes

@kathy-t kathy-t kathy-t approved these changes

@denis-yuen denis-yuen denis-yuen approved these changes

@coverbeck coverbeck coverbeck approved these changes

Assignees

@hyunnaye hyunnaye

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hyunnaye @coverbeck @denis-yuen @kathy-t @svonworl