-
Notifications
You must be signed in to change notification settings - Fork 34
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
workspace: add file search #212
Conversation
b12aa83
to
e9ea0f5
Compare
06f3b16
to
6dc01b4
Compare
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.
Nice work, everything works as expected without any issues!
- I think we could update the
readme
withyarn install
instead ofnpm install
. Since we haveyarn.lock
andpackage-lock.json
it could be confusing which one we should use? - Search bar placeholder text is hard to read since the color is barely different from the background if not focused. If it's the same for you, we could create a mini issue and to fix it later?
reana-ui/src/actions.js
Outdated
return async (dispatch) => { | ||
dispatch({ type: WORKFLOW_FILES_FETCH }); | ||
const nameSearch = search ? JSON.stringify({ name: [search] }) : search; |
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.
Very minor suggestion: since we have the same logic in line 246, we could extract it into a small function
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've created a util function and added some basic tests as this is very likely to change in the future if e.g. we want to support multiple-term search ("Search for both roofit and snakemake terms")
@@ -2,20 +2,20 @@ | |||
-*- coding: utf-8 -*- | |||
|
|||
This file is part of REANA. | |||
Copyright (C) 2020 CERN. | |||
Copyright (C) 2020, 2021, 2022 CERN. |
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.
Very minor: copyright year was not updated in most of the other touched files
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.
Updated, thanks!
Did you manage to find a nice way to keep this updated in VSCode?
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.
Not yet, is seems there are no extensions which would do exactly this task in VSCode unfortunately. However there are extensions like 'Run on Save' which could trigger custom task like this after saving a file, but I haven't tried to implement it.
@@ -3,11 +3,11 @@ | |||
"version": "0.8.0", | |||
"private": true, | |||
"dependencies": { | |||
"axios": "latest", | |||
"axios": "^0.24.0", |
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.
Nice to see this! 👍
6dc01b4
to
0ca1032
Compare
Good catch, done in a new commit!
Mmm it's true that is very light but I still can see it. This is the default color, check SUI-React Input. I've created a new commit making it a bit darker, let me know what you think. |
That was quick! Thanks, it looks way better now I think |
reana-ui/src/util.js
Outdated
* @param {String} term Search term | ||
* @returns term format expected by the API. | ||
*/ | ||
export function parseSearch(term) { |
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.
Minor question about the naming. Does this function really parses
search input term? As I understand it produces a query param, so this function call could be misleading to someone. Perhaps getSearchParam
or something like that would be more meaningful? WDYT?
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.
Yeah, I wasn't very sure either, I followed other function naming but the logic is not really the same. getSearchParam
is a bit better but I'm still not convinced, maybe encodeSearch
?
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.
One of the hardest problems in CS..
encodeSearch
sounds nice to me! Though it might be mixed with encoding of strings. Another solution would be simply formatSearch
?
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.
Fair enough 👍
- Extract out common `Search` component - Add search param to workspace endpoint closes reanahub#211
- Upgrade `node-sass` package, responsible of some vulnerabilites listed in https://github.com/reanahub/reana-ui/security/dependabot
- Use `yarn` instead of `npm`, as this is what we use to build the production docker image.
0cb1730
to
2406b62
Compare
closes #211
closes #204
Screen.Recording.2022-01-12.at.4.38.11.PM.mov
Test: