datajoint / sci-viz

Generic visualization framework for building dashboarding capabilities for DataJoint pipelines.
https://datajoint.com/docs/core/sci-viz
MIT License
6 stars 12 forks source link

Refactor table component to use ant design #52

Closed jverswijver closed 1 year ago

jverswijver commented 2 years ago

some of the work has already been completed on my fork. @zitrosolrac is now taking over. here is a ordered list of the things that need to still be added for the table component to be considered complete:

If all are met before the release we can deprecate the old table. Lets try to keep technical discussions about the tasks on the above list in this issue for documentation.

zitrosolrac commented 2 years ago

Hey @jverswijver, i was trying to swap out the old TableView component with the new DjTable component but i'm not sure if i'm bringing in the correct props. I'm missing height and restrictionList but i'm not sure if the formatting of restrictionList is correct for DjTable.

jverswijver commented 2 years ago

What props does the new table need vs the old table?

zitrosolrac commented 2 years ago

<TableView token={this.props.jwtToken} route='/query2' tableName='Example table (conditional row coloring)' updateRestrictionList={this.updateRestrictionList} updatePageStore={this.updateStore}/> is what old component is using and i'm not sure what the new needs but in the constructor it has data and dataAttributes.

jverswijver commented 2 years ago

for height use whatever the markdown component has in the page you are trying to insert the new table onto. for restriction list use this code snippet restrictionList={[...this.state.restrictionList]}

jverswijver commented 2 years ago

restriction list comes from the page, it represents the list of query params passed in through the url.

zitrosolrac commented 2 years ago

i'm not getting an error with the component anymore but now there is an error coming from the updateRestrictionList method within the setState method. Type 'URLSearchParams' is missing the following properties from type 'string[]': length, pop, push, concat, and 23 more.

jverswijver commented 2 years ago

Try giving it an empty list for now, instead of the code snippet i sent.

zitrosolrac commented 2 years ago

Also, is there a reason changes to Table.jsx aren't being tracked by VS code? I'm not seeing the file under the Source Control menu for Git.

zitrosolrac commented 2 years ago
Screen Shot 2022-07-19 at 3 09 40 PM

Looking good?

jverswijver commented 2 years ago

Yes, all the files in the pages directory are dynamically created from the spec sheet. This means that the files in there are different depending on the config you give sci-viz. The files that you have in there now have been generated based off of the spec we use for testing called test_spec.yaml under the test directory.

jverswijver commented 2 years ago

Also it looks like the ant table is paging the data that is sent on the frontend right now (like the ibl navigator used to do). We want the paging to be full controlled from the backend.

zitrosolrac commented 2 years ago

where would the backend file be in the repo? also, so the ant table paging won't be necessary as you mentioned before?

jverswijver commented 2 years ago

The table is paging the data in the frontend right now (is what it looks like), pharus has the ability to page in the backend. We want to implement the table paging using the backend paging functionality. I would look through pharus a bit to understand how it works.

zitrosolrac commented 2 years ago

Hey @jverswijver, the request in the backend that's supplying the data into the DjTable.jsx table is within the server.py file within the pharus/pharus submodule directory right?

jverswijver commented 2 years ago

It is in component_interface.py specifically the dj_query_route method of the TableComponent class. You can see that it checks for limit and page those are the two query parameters pharus is looking for when doing backend paging.

zitrosolrac commented 2 years ago

Should i setting those query parameters within the DjTable fetch requests and making sure pharus is handling them on the backend?

jverswijver commented 2 years ago

Yes, pharus should already handle them properly since that is how the current table works. You just need to implement it in the same way as the current table.

zitrosolrac commented 2 years ago

How does the currentPageNumber prop get updated within the TableView component? I don't see an increment method within the component.

jverswijver commented 2 years ago

it gets changed with the setPageNumber method that gets passed to the TableContents component.

zitrosolrac commented 2 years ago

Since we want to use Ant Table, I'm not sure how i could increment the page number since I can't apply methods within the Ant Table like the TableView is doing.

zitrosolrac commented 2 years ago

I'm viewing this site https://ant.design/components/pagination/ and I see there is this method called onChange and I remember you saying that I could use the built in methods within the Ant Table.

jverswijver commented 2 years ago

So the antd table has one of those paginators built into it, you want to give that paginator an on change method to set what page you are on and also how many records to show(called limit in pharus).

zitrosolrac commented 2 years ago

maybe we can set up a call tomorrow to go over the details behind this onChange implementation?