Issue/96 advanced search
Changes introduce an "Advanced Search" Dropdown to the IDA page. Dropdown currently consists of Author, Runtime Platform and UI Type
Closes issue #96
Merge request reports
Activity
added 1 commit
- 4dd9d830 - Added classname to main textfields, apply style to classes in CSS
requested review from @kliffen
Overall, it seems to work. I do have some feedback. Don't be afraid to reach out to me if you need help with some of it!
- The interactive page seems to load very slow. This probably has nothing to do the front-end code, but more with the back-end. I will probably create a separate issue for it.
- It would be cool if the advanced searchbox stays open when you click the search button.
- There are some issues with the code:
- React uses
classNameinstead ofclassto assign a css class (these were not from your commits, but can be safely updated) - Instead of
React.useStateyou can useuseStatedirectly (it is imported at the top) - You can combine the
clickedAdvancedandshowAdvancedtogether. Only perform an advanced search when the Box is shown -
const [defaultWorkflow] = ["https://github.com/ESAP-WP5/binder-empty"];can just be assigned directly instead of using an array. - There are a lot of state variables, a common pattern is to group them in an object (based on functionality):
- React uses
const [advSearch, setAdvSearch] = useState({ searchAuthor: "", searchType: "", // ... etc }); // An update would like like this: setAdvSearch(prev => { return { ...prev, // this will fill the object with the previous state searchAuthor: "updated value", // other attributes that need updating } });A tip regarding some fixes: take a look at the console output. React will warn about certain issues (there are a lot of warnings (mostly for other files!), but for Interactive.js there are the following
Line 1:17: 'createContext' is defined but never used no-unused-vars Line 1:32: 'useState' is defined but never used no-unused-vars Line 3:35: 'Alert' is defined but never used no-unused-vars Line 11:70: 'batchsystemsURL' is assigned a value but never used no-unused-vars Line 11:87: 'setBatchsystemsURL' is assigned a value but never used no-unused-vars Line 42:8: React Hook useEffect has missing dependencies: 'defaultWorkflow', 'setList_of_workflows', and 'setWorkflowURL'. Either include them or remove the dependency array react-hooks/exhaustive-deps Line 53:8: React Hook useEffect has missing dependencies: 'setIdaSystemURL' and 'setList_of_idaSystems'. Either include them or remove the dependency array react-hooks/exhaustive-deps Line 84:27: Expected '===' and instead saw '==' eqeqeq Line 103:49: Unexpected mix of '&&' and '||' no-mixed-operators Line 103:121: Unexpected mix of '&&' and '||' no-mixed-operators Line 103:121: Unexpected mix of '||' and '&&' no-mixed-operators Line 104:53: Unexpected mix of '||' and '&&' no-mixed-operators Line 104:129: Unexpected mix of '||' and '&&' no-mixed-operators Line 105:51: Unexpected mix of '||' and '&&' no-mixed-operators Line 105:125: Unexpected mix of '||' and '&&' no-mixed-operators Line 106:60: Unexpected mix of '||' and '&&' no-mixed-operators Line 106:143: Unexpected mix of '||' and '&&' no-mixed-operators Line 107:56: Unexpected mix of '||' and '&&' no-mixed-operators Line 162:33: Expected '!==' and instead saw '!=' eqeqeq Line 166:35: Expected '!==' and instead saw '!=' eqeqeq Line 170:35: Expected '!==' and instead saw '!=' eqeqeq Line 259:7: The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md jsx-a11y/anchor-is-validEspecially the hook warnings can point to hard to debug errors, although they are not caused by your changes, it might be nice to clean them up a bit ;)
- Resolved by Klaas Kliffen
Thanks for the review and suggestions @kliffen . The last commit should have fixed most of the issues you mentioned. If you get a chance could you have another look and check that the changes are good enough, or if there are any other specific changes that would be good to apply before merging this back to main?
I have also noticed that the page is slow to load, this is probably due to the request to Zenodo/OSSR to collect the records. One possible fix would be that the main page doesn't show any results until a search is made.
- Last reply by Klaas Kliffen
mentioned in commit cdd36be7
mentioned in issue esap-general#96 (closed)
