-
Notifications
You must be signed in to change notification settings - Fork 16
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
DOCK-1525: reordering version dropdown in my workflow #1720
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #1720 +/- ##
===========================================
- Coverage 40.74% 40.56% -0.19%
===========================================
Files 349 349
Lines 10646 10676 +30
Branches 2728 2736 +8
===========================================
- Hits 4338 4331 -7
- Misses 4068 4103 +35
- Partials 2240 2242 +2
... and 7 files with indirect coverage changes 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 in Codecov by Sentry. |
Hi, Nayeon, thanks for the work on this. Could you please add a few comments at key spots in the code, it will help me to follow it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool!
Some observations about using the control, that you might be constrained by what customization the control allows:
- You have to click on the selection. For example, type
develop
, assuming there is a match; it only showsdevelop
, but you then have to click on it. Could be just me, I try to use the mouse as little as possible. - If you type a matching version, then press tab (no click), you end up in a state where it doesn't display anything. Screenshot attached.
- You could argue this both ways, but I don't like how it "remembers" your last search. I type develop, select it, now I want to look for master; it's still showing what I typed for
develop
beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question
THIRD-PARTY-LICENSES.csv
Outdated
@@ -805,6 +805,7 @@ | |||
"make-fetch-happen@10.2.1","ISC","https://github.com/npm/make-fetch-happen" | |||
"make-fetch-happen@5.0.2","ISC","https://github.com/zkat/make-fetch-happen" | |||
"marked@4.2.3","MIT","https://github.com/markedjs/marked" | |||
"mat-select-search@1.2.21","Custom: https://github.com/shafi-sahal/MatSelectSearch","" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was a big thread , didn't fully read through.
Did you consider https://github.com/bithost-gmbh/ngx-mat-select-search which seems to be more popular?
(this is a discussion, I know little about the two aside from seeing both in the thread and this one seems to have more commits/stars)
MIT License also a little more clearly open source than "custom"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried ngx-mat-select-search but I couldn't get it to compile due to various module errors like ERROR in ./node_modules/ngx-mat-select-search/fesm2020/ngx-mat-select-search.mjs 842:108-124 export 'MatIconButton' (imported as 'i4') was not found in '@angular/material/button' (possible exports: MatAnchor, MatButton, MatButtonModule)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try version 5?
https://github.com/bithost-gmbh/ngx-mat-select-search#compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, i'll try that out right now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to ngx-mat-select-search as it seemed to resolve Charles' issues above
Thank you for this! I switched over to ngx-mat-select-search following Denis' comment and it seems to resolve this issue. I will rerequest review after verifying all tests pass in my latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, so if I understand correctly, the build issues was the bad cache in the end?
Yea, I changed the version on package.json and package-lock.json and that seems to fix it.. Very odd |
I think what happened is the first time the cache was generated, it was messed up and every subsequent build it pulled the messed up cache until you busted the cache with the change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running it locally, and it looks and acts a lot nicer than the previous iteration. I did hve one question though that I left inline.
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment with one more question, but assuming that's resolved, approved.
Description
This PR adds a search feature onto the version dropdown in My Workflows and reorders the dropdown to alphabetical order.
Library used: https://www.npmjs.com/package/ngx-mat-select-search
Before change:

After change (search bar and alphabetical sorting):



Review Instructions
Review that the newly created integration test pass and the search feature correctly filters through the different versions of a workflow.
Issue
dockstore/dockstore#3722
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!
npm run build
markdown-wrapper
component, which does extra sanitizationnpm audit
and ensure you are not introducing new vulnerabilities