ral-facilities / datagateway

DataGateway is a portal that supports discovery and access large science facilities data. It is developed as a plugin for SciGateway
Apache License 2.0
9 stars 5 forks source link

Create card/grid view for generic entities #128

Closed agbeltran closed 4 years ago

agbeltran commented 4 years ago

Create a component to visualise different entities in a grid/card view. This component will be in the datagateway-dataview package.

This card view needs to be paginated.

We will customise it for specific entities such as investigations and datasets (and there will be separate issues for those).

This relates to https://github.com/ral-facilities/datagateway-userstories/issues/110

Acceptance criteria

GoelBiju commented 4 years ago

The browse datagateway user story which shows some examples of card/grid views posted by @agbeltran.

GoelBiju commented 4 years ago

So initial plan is to create a single card component and move onto creating multiple.

This will involve:

An initial mockup of a single card is the following:

image

The whole card is in a flexbox but we only control the specifics of the layout for the items that are to the right of the image in the card. This is because we need to achieve two things with the flexbox:

This is the initial Balsamiq mockup that I am working of for the whole datagateway-dataview and will be changed when required.

Some specific resources to learning flexbox's:

GoelBiju commented 4 years ago

This can be updated when needed but other components of the CardView component include:

We will also need to make this component generic, similar to how the Table component works and just provide data to it and customise how it can be displayed for different entities (investigation/datasets).

GoelBiju commented 4 years ago

At the moment I can start working with the investigation data. Similar to the Table component, the CardView should be able to accept props such as data, onClick, sort/onSort and check/onCheck in order to be customisable for different entities (i.e. investigations/datasets).

This CardView can then be used in custom card components e.g. InvestigationCardView/DatasetCardView but this will require more customisable in terms of layout.

The current priority is to:

GoelBiju commented 4 years ago

Ignore https://github.com/ral-facilities/datagateway/commit/6fef79a233fc93da79e6d4df9d52ad0a09e882f0 as it is only related with #162 (this commit has been renamed for that branch).

GoelBiju commented 4 years ago

Ongoing items to fix:

GoelBiju commented 4 years ago

Additional features for the Card and CardView (arising from sprint planning and remaining tasks):

GoelBiju commented 4 years ago

The updated mockup for the whole CardView includes the add/remove from cart buttons and search box:

image

GoelBiju commented 4 years ago

To accommodate different data and to show less information for certain entities, we may need to check that the card layout works well when you have less information provided i.e. when there are no tags, no further information.

Also the card needs to support for entities which may not be items to add to cart but more links to further information, so the add/remove to/from cart may not be present. These varying layouts may be required for a variety of entities for ISIS and DIAMOND.

GoelBiju commented 4 years ago

A feature that needs to be implemented is filtering. At the moment querying for ISIS means that we cannot use the distinct option to get only the data we need, meaning all data is fetched when we need we need to know all the values possible for filtering options.

To get around this we can provide the option to not use a paginated fetch of the data (so not using the offsetParams to fetch data) and then just assume the view has passed in all the data available. As a result we do not fetch all the data again. This means we would just have to provide the option to slice data or query the API again to fetch data based on offsetParams. A check maybe required before doing the slice (so the stopIndex is not assumed to be greater than the actual data length) and make sure the count we use is depending on if fetching data via pagination has been flagged so we can choose whether to use totalDataCount or the data.length.

As @louise-davies mentioned this is a bug and once this issue has been fixed in the future we can provide the option to just provide this via pagination only. The custom view at the moment will need to pass in an array of all the data returned for the specific property (from the whole data and not just the paginated data on the page) otherwise the filtering for that property cannot be done.

GoelBiju commented 4 years ago

A feature that needs to be implemented is filtering. At the moment querying for ISIS means that we cannot use the distinct option to get only the data we need, meaning all data is fetched when we need we need to know all the values possible for filtering options.

To get around this we can provide the option to not use a paginated fetch of the data (so not using the offsetParams to fetch data) and then just assume the view has passed in all the data available. As a result we do not fetch all the data again. This means we would just have to provide the option to slice data or query the API again to fetch data based on offsetParams. A check maybe required before doing the slice (so the stopIndex is not assumed to be greater than the actual data length) and make sure the count we use is depending on if fetching data via pagination has been flagged so we can choose whether to use totalDataCount or the data.length.

As @louise-davies mentioned this is a bug and once this issue has been fixed in the future we can provide the option to just provide this via pagination only. The custom view at the moment will need to pass in an array of all the data returned for the specific property (from the whole data and not just the paginated data on the page) otherwise the filtering for that property cannot be done.

Allowed both options in https://github.com/ral-facilities/datagateway/commit/c89ee8ed3edc9ae92a6f0aebba3d5e4732ce070a. We now need to provide filtering methods to put data in the state to get filtering information.

GoelBiju commented 4 years ago

Once the card view has been implemented, the main components need to be moved into datagateway-common.

GoelBiju commented 4 years ago

I have removed the counting of the filtering options in https://github.com/ral-facilities/datagateway/commit/b212c35fa8683634c51c4f0c73c464763c471801. This is due to the fact that the API currently does not support fetching a single property for all data. Without this, we will have to fetch all data and then we would not require pagination. If this is added then we can add back in the count which is similar to what is mocked up by PanOSC.

An issue has been created to track this feature (#265) so it can be added in the future.

GoelBiju commented 4 years ago

Mentioned by @louise-davies, there are already actions which allow for filtering i.e. filterTable which adds a value to the existing Filter object in state. We will need to add support for string and number arrays so that multiple items can be added to support filtering (using in for where in the API query).

The following needs to be done in order to adjust filtering to work using the existing state:

GoelBiju commented 4 years ago

A feature that needs to be implemented is filtering. At the moment querying for ISIS means that we cannot use the distinct option to get only the data we need, meaning all data is fetched when we need we need to know all the values possible for filtering options.

To get around this we can provide the option to not use a paginated fetch of the data (so not using the offsetParams to fetch data) and then just assume the view has passed in all the data available. As a result we do not fetch all the data again. This means we would just have to provide the option to slice data or query the API again to fetch data based on offsetParams. A check maybe required before doing the slice (so the stopIndex is not assumed to be greater than the actual data length) and make sure the count we use is depending on if fetching data via pagination has been flagged so we can choose whether to use totalDataCount or the data.length.

As @louise-davies mentioned this is a bug and once this issue has been fixed in the future we can provide the option to just provide this via pagination only. The custom view at the moment will need to pass in an array of all the data returned for the specific property (from the whole data and not just the paginated data on the page) otherwise the filtering for that property cannot be done.

As an update to this whilst discussing with @louise-davies, in the future, the distinct filter will be working for ISIS endpoints and also the issue relating to fetching data with a nested properties this means we can use the distinct filter for getting filters like INVESTIGATIONINSTRUMENT.INSTRUMENT.NAME and then pagination will work for fetching data with nested properties with where.

At the moment the code to do fetching without pagination will be removed and there will be no filtering on the ISIS tables.

GoelBiju commented 4 years ago

There is an active issue regarding having a feature for showing the count of the specific filter options. Previously this was demonstrated by fetching the distinct filters and then counting via client-side (for a lot of data this may not be feasible).

GoelBiju commented 4 years ago

Add the following filters to DLS (DIAMOND) pages:

GoelBiju commented 4 years ago

We need to ensure that filters and sorts queries are supported by all the table components:

GoelBiju commented 4 years ago

Tables now support filters and sorts, although switching between views keeps the same data and does not clear the data appropriately.

GoelBiju commented 4 years ago

There is now a requirement for an advanced search which allows the user to specifically filter the information on the cards (similar to the table view) e.g. TITLE, DESCRIPTION, any of the further information (including dates). This can be provided through smaller text boxes which allows for date selection as well.

GoelBiju commented 4 years ago

Following a code review of this branch, a couple of features and issues were noted:

GoelBiju commented 4 years ago

We also need to add in forward support for search and sort queries (for when they are implemented):

GoelBiju commented 4 years ago

Add tests for the following:

GoelBiju commented 4 years ago

An issue found is that after applying a custom filter the pagination component does not re-render with new values. This leads to the possibility that you can click on a page number which is incorrect and the card view would not show any data.

GoelBiju commented 4 years ago

Add icons support in line with the suggested UI improvements. Icons won't be added to title and description but will be added to the information area:

GoelBiju commented 4 years ago

An issue that is still persistent is that data is fetched twice from the API (due to the count updating later on?). So for all count requests we get the data being fetched twice:

Looking at Redux we can see that clearData gets called twice and fetch data is called again. Most likely due to a state update meaning useEffects are run again and the parameters still allow for another request to be made (check loadedData?).

GoelBiju commented 4 years ago

Add icons support in line with the suggested UI improvements. Icons won't be added to title and description but will be added to the information area:

  • [x] important: icons are supported on information in card via the card view.

Icons can be changed, if required, after a sprint review.

GoelBiju commented 4 years ago

An initial data fetch with a max results query parameter results in the limit being calculated as -1 and therefore the data request fails.

GoelBiju commented 4 years ago

An issue that is still persistent is that data is fetched twice from the API (due to the count updating later on?). So for all count requests we get the data being fetched twice:

  • [ ] important: data gets fetched twice after a count request; we only want one count and one data request (with all changes in sort/filters/custom filters/max results).

Looking at Redux we can see that clearData gets called twice and fetch data is called again. Most likely due to a state update meaning useEffects are run again and the parameters still allow for another request to be made (check loadedData?).

Furthermore if we are navigating from a card view which already had a loaded count, then the previously used count is also fetched twice:

GoelBiju commented 4 years ago

An initial data fetch with a max results query parameter results in the limit being calculated as -1 and therefore the data request fails.

  • [x] important: Initial data fetch does not occur with a limit of -1.

The num pages and start/stop index was strictly checking for -1, this has been changed to only fetch data for greater values.

GoelBiju commented 3 years ago

An issue that is still persistent is that data is fetched twice from the API (due to the count updating later on?). So for all count requests we get the data being fetched twice:

  • [ ] important: data gets fetched twice after a count request; we only want one count and one data request (with all changes in sort/filters/custom filters/max results).

Looking at Redux we can see that clearData gets called twice and fetch data is called again. Most likely due to a state update meaning useEffects are run again and the parameters still allow for another request to be made (check loadedData?).

This seems like it is still an issue within the CardView component. This should be investigated and we need to ensure that the CardView component does not produce duplicate requests.

GoelBiju commented 3 years ago

An issue that is still persistent is that data is fetched twice from the API (due to the count updating later on?). So for all count requests we get the data being fetched twice:

  • [ ] important: data gets fetched twice after a count request; we only want one count and one data request (with all changes in sort/filters/custom filters/max results).

Looking at Redux we can see that clearData gets called twice and fetch data is called again. Most likely due to a state update meaning useEffects are run again and the parameters still allow for another request to be made (check loadedData?).

Furthermore if we are navigating from a card view which already had a loaded count, then the previously used count is also fetched twice:

  • [x] important: navigating between card views does not re-issue a count request from the previous card view (this is because it is still in state?)

This no longer seems to be an issue and I do not see duplicate count requests.

GoelBiju commented 3 years ago

An issue that is still persistent is that data is fetched twice from the API (due to the count updating later on?). So for all count requests we get the data being fetched twice:

  • [ ] important: data gets fetched twice after a count request; we only want one count and one data request (with all changes in sort/filters/custom filters/max results).

Looking at Redux we can see that clearData gets called twice and fetch data is called again. Most likely due to a state update meaning useEffects are run again and the parameters still allow for another request to be made (check loadedData?).

This seems like it is still an issue within the CardView component. This should be investigated and we need to ensure that the CardView component does not produce duplicate requests.

If this issue still exists, then it should hopefully be fixed through the refactor of using react-query for table and card views in #701.