Health-Informatics-UoN / Carrot-Mapper

Carrot: Convenient And Reusable Rapid Omop Transformer.
https://carrot4omop.ac.uk
MIT License
12 stars 3 forks source link

Implement NextJS to Scan Reports Value view #722

Closed AndrewThien closed 1 month ago

AndrewThien commented 1 month ago

Changes

This PR implemented NextJS to build the page Values of the selected Scan report.

Closes #681 Closes #677

Screenshots

Screenshot 2024-05-28 at 11 47 10

Checks

Important: please complete these before merging.

github-actions[bot] commented 1 month ago
Tests Skipped Failures Errors Time
25 1 :zzz: 1 :x: 0 :fire: 4.488s :stopwatch:
spco commented 1 month ago

Is it possible to set the default number of rows to a larger number on this view? 10 per page will almost always be annoyingly small, when these fields can contain >1000 values. 25 might be a more suitable default and still give good performance. @erummas or @armando-mv might be able to weigh in with a default value? (I know the user can change this at the bottom of the page, but the users will prefer not to have to do that every time, I'm sure).

AndrewThien commented 1 month ago

Is it possible to set the default number of rows to a larger number on this view? 10 per page will almost always be annoyingly small, when these fields can contain >1000 values. 25 might be a more suitable default and still give good performance. @erummas or @armando-mv might be able to weigh in with a default value? (I know the user can change this at the bottom of the page, but the users will prefer not to have to do that every time, I'm sure).

About this one, I have changed the value to 25, as requested, but was not able to test it on my end (because I don't have the test data having more than 25 values in one field). So can you please check on your end to see if the change is well implemented, @spco ? Thanks

AndyRae commented 1 month ago

About this one, I have changed the value to 25, as requested, but was not able to test it on my end (because I don't have the test data having more than 25 values in one field). So can you please check on your end to see if the change is well implemented, @spco ? Thanks

This change works for me locally - I think 25 is a nice default for now.

AndyRae commented 1 month ago

It would be nice to preserve the tooltips also, for example:

Screenshot 2024-05-28 at 12 37 14

The reason being - they give a readable example of what the (V/M/R) actually mean.

AndrewThien commented 1 month ago

It would be nice to preserve the tooltips also, for example:

Screenshot 2024-05-28 at 12 37 14

The reason being - they give a readable example of what the (V/M/R) actually mean.

@AndyRae Yeah, I tried to add Tooltip from ShadCN but for some reasons, it behaved very strange. So I decide to use another package which is react-tooltip. The demo is below. Hope it is fine. Please comment on its styling as well Screenshot 2024-05-28 at 15 03 01

AndyRae commented 1 month ago

@AndyRae Yeah, I tried to add Tooltip from ShadCN but for some reasons, it behaved very strange. So I decide to use another package which is react-tooltip. The demo is below. Hope it is fine. Please comment on its styling as well !

That's fine given it's what Shadcn uses, so we're fairly consistent at least. And looks good to me!

AndrewThien commented 1 month ago

I also modified the pagination buttons' styling, making them neater a bit. Please re-review, @AndyRae

AndrewThien commented 1 month ago

Please also re-review, @spco. Thanks!