pkpdapp-team / pkpdapp

A web application for modeling the distribution and effects of drugs.
BSD 3-Clause "New" or "Revised" License
10 stars 3 forks source link

PKPDX-32 trial design changes #597

Open wniestroj opened 21 hours ago

wniestroj commented 21 hours ago

image

eatyourgreens commented 21 hours ago

Looks good. One comment about the UX, though. If I move through the protocol form from left to right, using Tab to move from field to field, then I can't easily jump to Add New Row when I'm finished. I think it would be better UX, and more accessible, to have Add New Row after the protocol dosing form fields.

https://github.com/user-attachments/assets/5e41dcdd-45b9-49e5-adfd-961f39eafb47

codecov[bot] commented 20 hours ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.55%. Comparing base (c227f57) to head (4fd16dc). Report is 31 commits behind head on ui-enhancements.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## ui-enhancements #597 +/- ## =================================================== + Coverage 76.17% 76.55% +0.37% =================================================== Files 108 111 +3 Lines 5600 5651 +51 =================================================== + Hits 4266 4326 +60 + Misses 1334 1325 -9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sonarcloud[bot] commented 8 hours ago

Quality Gate Passed Quality Gate passed for 'pkpdapp'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sonarcloud[bot] commented 8 hours ago

Quality Gate Failed Quality Gate failed for 'pkpdapp-team_pkpdapp_frontend'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

wniestroj commented 8 hours ago

@eatyourgreens Regarding navigation using tab - I feel like that is a fair point, however, user can always use 'shift' + 'tab' to move through inputs and buttons the other way.

I think that whether the Add Row button will be placed above the list or below the list, there will always be cases where navigating to it via keyboard will be challenging - when there will be list of 10 rows:

  1. there might be a case where first thing a user would like to do is add row - placement at the top makes it easy, placing it at the bottom would mean clicking tab ~70 times
  2. there might be a case where user wants to read the data to look for specific value before adding a row - then placement at the top means they have two ways (either go further with tab through main navigation and groups or go back using shift + tab through the whole table), but placement at the bottom would make this case easy

I will pass your point to Michael and Martin and will discuss what is the best solution

martinjrobins commented 6 hours ago

Looks good. One comment about the UX, though. If I move through the protocol form from left to right, using Tab to move from field to field, then I can't easily jump to Add New Row when I'm finished. I think it would be better UX, and more accessible, to have Add New Row after the protocol dosing form fields.

I think on a keyboard that button is always going to be inconvenient. Sometimes you'll be at the bottom of the table and want to add a row, sometimes you'll be at the top. I think the most likely case is that you'll be at the top and want to add a row, so I'd keep it at the top. Mind you, then you have to tab right down to the bottom to edit the values :( I would say that tab navigation through a table is always going to be a bad idea, surely you want to use arrow keys (left/right for columns, up/down for rows)? Or a keyboard shortcut for adding a new row, I notice github uses a lot of shortcuts: https://docs.github.com/en/get-started/accessibility/keyboard-shortcuts

eatyourgreens commented 6 hours ago

I think pressing Tab after filling out a form field is pretty common behaviour, particularly for filling out long forms. Off the top of my head, I can't think of a browser, or desktop app, that uses cursor keys to navigate forms. Having said that, screen readers do support cursor keys for navigating data table columns and rows, in tables mode.

wniestroj commented 6 hours ago

Using up and down to navigate through rows might not be feasible in that case, as those buttons are used to either increment/decrement number values or navigate through dropdown options in selects

martinjrobins commented 4 hours ago

Using up and down to navigate through rows might not be feasible in that case, as those buttons are used to either increment/decrement number values or navigate through dropdown options in selects

Its feasible, but might be more expensive if its just for the paid version: the MUI datagrid supports navigation with arrow keys: https://mui.com/x/react-data-grid/accessibility/#tab-sequence.

I would suggest we just leave the button where it is for the moment, but in the longer term I think we'll need better keyboard navigation for (all) the tables in the app.