Initial front-end restructure and code re-design #381
Conversation
There was a problem hiding this comment.
Minor things that I picked up.
Otherwise nice stuff in there: cleanup & testing 👍
Experienced a problem when trying to run SCope:
> [email protected] dev ~/SCope/master
> cross-env NODE_ENV=development NODE_TYPE=local webpack-dev-server --hot
~/SCope/master/node_modules/terser-webpack-plugin/node_modules/p-limit/index.js:30
} catch {}
^
SyntaxError: Unexpected token {
at new Script (vm.js:51:7)
at createScript (vm.js:136:10)
at Object.runInThisContext (vm.js:197:10)
at Module._compile (internal/modules/cjs/loader.js:618:28)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:665:10)
at Module.load (internal/modules/cjs/loader.js:566:32)
at tryModuleLoad (internal/modules/cjs/loader.js:506:12)
at Function.Module._load (internal/modules/cjs/loader.js:498:3)
at Module.require (internal/modules/cjs/loader.js:598:17)
at require (internal/modules/cjs/helpers.js:11:18)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] dev: `cross-env NODE_ENV=development NODE_TYPE=local webpack-dev-server --hot`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] dev script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! ~/.npm/_logs/2020-09-18T12_47_32_043Z-debug.lo
even after removing node_modules and package-lock.json
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
c226293
to
8adf03a
Compare
| @@ -222,28 +227,42 @@ def get_final_feature_and_type( | |||
| return features, feature_types | |||
|
|
|||
|
|
|||
| def get_search_results(search_term: str, loom: Loom, data_hash_secret: str) -> Dict[str, List[str]]: | |||
| def get_search_results(search_term: str, category: str, loom: Loom, data_hash_secret: str) -> List[CategorisedMatches]: | |||
There was a problem hiding this comment.
The lru_cache was removed from GServer. Did you consider putting it back here?
I know it was added a while ago, before the search refactor, so might be worth testing the search times in the current codebase to see if it is still worth it.
There was a problem hiding this comment.
No I didn't. Have you got a test load? lru_cache makes me very nervous unless it's super obvious that it's a pure function. This is absolutely not a pure function!
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
fc668ca
to
67c4153
Compare
|
Add PR summary points to documentation (conribution manual??) |
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
9084c70
to
463e678
Compare
|
/!\ I noticed that |
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
ef5f127
to
920af81
Compare
Reproductions steps:
|
There was a problem hiding this comment.
In general, nice cleanup and refactoring! I noticed a few bugs across the app tough:
- Running the app using Node
v10.22.1fails with a huge stack trace. I was able to run it usingv11.15.1. So I suggest we use.nvmrcfile to properly state which Node version this app requires. - The markers table should not be displayed (actually not exist) when querying for
All Clustersin theGenepage.
- When switching from loom, the viewer is not reloaded with the embedding
- After having switched from loom, colors are not properly assigned to dots anymore:
- To reproduce:
- Load one loom, search for
All Clusters - Load other loom, clear search box and search for
All Clusters
- Load one loom, search for
- To reproduce:
- When switching from embeddings/coordinates, the scatterplot is not updated.
- All annotations page is empty/blank while loom contains community annotations
- Some viewers are missing in the compare tab. When switching to the
Comparetab only 2 are shown. - Compare tab break when performing a DnD of one of the annotation onto a viewer.
| <div className='scope-row'> | ||
| <FileReaderInput | ||
| as='binary' | ||
| id='my-file-input' | ||
| onChange={this.selectLoomFile.bind(this)}> | ||
| <Input | ||
| label='File to be uploaded:' | ||
| labelPosition='left' | ||
| action={{ | ||
| color: this.state.uploadLoomFile | ||
| ? 'grey' | ||
| : 'orange', | ||
| content: 'Select a file...', | ||
| }} | ||
| fluid | ||
| placeholder={this.state.uploadLoomFileName} | ||
| error={this.state.uploadError} | ||
| /> | ||
| </FileReaderInput> | ||
| <Button | ||
| className='scope-row-element' | ||
| color={ | ||
| this.state.uploadLoomFile | ||
| ? 'orange' | ||
| : 'grey' | ||
| } | ||
| onClick={this.uploadLoomFile.bind(this)} | ||
| disabled={ | ||
| !this.state.uploadLoomFile || | ||
| this.state.uploadLoomProgress > 0 | ||
| }> | ||
| {' '} | ||
| Upload! | ||
| </Button> | ||
| </div> | ||
| <div> | ||
| <span>Upload progress:</span> | ||
| <span> | ||
| <Progress | ||
| percent={this.state.uploadLoomProgress} | ||
| error={this.state.uploadError} | ||
| indicating | ||
| progress | ||
| disabled></Progress> | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
This is not crucial but to improve ux/ui, we could remove upload progress and only show the progress when this.state.uploadLoomProgress > 0 and < 100.
There was a problem hiding this comment.
I'm not quite sure what you mean. Isn't that the case now?
src/components/AppSidebar.jsx
Outdated
| @@ -332,14 +331,10 @@ class AppSidebar extends Component { | |||
| return ( | |||
| <Sidebar | |||
| as={Menu} | |||
| animation='push' | |||
| animation='overlay' | |||
There was a problem hiding this comment.
After playing around a bit, I'm really inclined to side with @dweemx here. It feels very "broken" as an overlay. Mainly because it isn't intuative the the viewers are hidden especially when the sidebar is interacted with so much. Switching between looms or changing coordinates is done very regularly and to have to reopen/close the sidebar for this time is irritating.
|
@dweemx wrt an |
|
@dweemx Can you elaborate on what "switch from loom" means? |
You mean not having a |
No I mean it's a users fault if they use an EOL node version. So why have an |
Because the user will be sure that the software will work with that version of Node. |
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
d57da8c
to
ade6360
Compare
| presets: [ | ||
| [ | ||
| '@babel/preset-env', | ||
| { | ||
| targets: { | ||
| esmodules: true, | ||
| }, | ||
| }, | ||
| ], | ||
| '@babel/preset-react', | ||
| ], | ||
| plugins: [ | ||
| [ | ||
| '@babel/plugin-proposal-class-properties', | ||
| { loose: true }, | ||
| ], | ||
| ['@babel/plugin-proposal-object-rest-spread'], | ||
| ['@babel/transform-runtime'], | ||
| ], |
There was a problem hiding this comment.
This seems redundant. Shouldn't these configs be picked up by babel.config.js ?
| if (!has(action.payload.field, draft)) { | ||
| (draft as State)[action.payload.field] = init( | ||
| action.payload.field | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is there a particular reason for its redundancy ? Why not put this into a function ?
* Move to typescript * Remove unnecessary and unused code * Upgrade semantic-ui-react to a more recent version * Re-organise some dependant code * Update packages and install immer
This can be used to only search for genes or regulons, etc. that match
the search query rather than everything that matches the search query.
* Add "MENU" text to the header menu button
* Default session mode should be "read"
* Remove dropdown on the FeatureSearchBox. Filter is imposed by the
page and is not user selectable.
* Adjust getFeatures gRPC/protocol buffers interface
* Modify the gRPC definition of getFeatures to be more convenient
* Fix a react warning
The warning triggered when trying to update the old state management
system using onResultSelect() while rendering. This is a no-no
so now it just calls the old state management update API.
This also moves remaining logic out of the component
* Do some error handling
* Some documentation
* Add basic Saga side-effect generation testing * Add Reducer type annotation (thanks @dweemx) * Use a clearer variable name for search results * Prefer `type` over `interface` * Use constants in App * The name "handleUpdateScatterPlot" is a more accurate than "handleUpdateTSNE" * Do not open the bundle analyzer by default * Bump semantic-ui-react version * use Sagas debounce() rather than throttle() * Remove unused import of lodash * Fix bug: Cannot model category as strict type, revert to string * Fix bug: Correct ordering in displayed search results
* Add some tests * Wire up redux hooks and fix tests * Fix "cancel" button in search box * Track cookie consent in redux
So that users immediately see where to load their data. Also centres content in the Welcome screen so still visible with the sidebar open.
* Remove all uses of abstract (in)equality
Abstract (in)equality is not type-safe and leads to
weird type coercion.
* eslint eqeqeq check
* Enforce all blocks should have brace delimiters
* Move definition of FEATURE_COLOURS constant
* Remove GeneSet page
* Use ts-loader instead of babel for typescript (ts-loader does type checking)
* Use pako for zlib compression
* Replace some uses of Lodash with Ramda
* Upgrade from Webpack 4 to Webpack 5
* Add another zipLists test
* Looping requests when selecting search result
"Unnannotated clustrer 0"
* UI disappears when ViewerSidebar is too tall
* Fix non-display of data in gene tab
* Add a contributing file
* Add a new contributing style rule
Concern for discoverability with the sidebar defaulted to hidden means that it should default to visible. The animation can make styling the rest of the app difficult. So this commit reduces/removes these complexities by making the sidebar permanently visible. * Finally remove the bin/ directory
GServer.py::getNextCluster had not been updated to match the change in results returned by get_search_results(). This issue is fixed along with having ViewerSidebar dispatch the Search 'SELECT' action. This action updates the search box ui.
MaybeJustJames
force-pushed
the
cleanup/373-frontend-stage3
branch
from
aad17cb
to
dd2a977
Compare
This branch begins a large-scale move of the app to redux and redux sagas for state management. It also restructures the app in a way I think is easier to navigate and think about.
Cleanup
fetch()And GProfiler previously introduced.jest-css-modules-transformto handle imports for CSS. Version is intentionally pinned as "4.0.2" breaks tests.LegacyAPInameAppcomponent in newMainandFullPageNotifycomponents.zipListstozip()a list of lists.default exportsCode structure and redesign
getFeature()requests and responsesgetFeature()requestsimmerto update Redux state.Resulttype similar to the backendResulttype.UI
Breaking changes and known issues
This PR breaks ViewerSidebar "Clustering Controls". Fixing this is out of scope for the PR, this should instead be fixed after @dweemx cleanup of the ViewerSidebar is merged.Compare tab break when performing a DnD of one of the annotation onto a viewer.