Convert SelectProposal to function component and other fixes
#1496
Conversation
| export function selectProposalAction(prop) { | ||
| return { | ||
| type: 'SELECT_PROPOSAL', | ||
| proposal: prop, | ||
| }; | ||
| } |
| return async (dispatch) => { | ||
| try { | ||
| await sendSelectProposal(number); | ||
| navigate('/'); |
There was a problem hiding this comment.
I don't think we need to navigate after selecting a proposal. The redirect to /login actually had no effect since the user was redirected right away to /datacollection.
| return ( | ||
| <Modal | ||
| show={show} | ||
| backdrop={showProposalsForm || 'static'} |
There was a problem hiding this comment.
The dialog doesn't have to be a modal when re-opened manually (i.e. when a proposal is already selected). This line allows the dialog to be dismissed by clicking on the backdrop in this situation.
| if (showProposalsForm) { | ||
| handleHide(); | ||
| } |
There was a problem hiding this comment.
This condition does the opposite: it makes the dialog truly modal when shown because no proposal is selected. It does so by preventing the dialog from being dismissed with Escape in this situation.
| data-default-styles | ||
| onClick={() => handleHide()} | ||
| > | ||
| {showProposalsForm ? 'Cancel' : 'Sign out'} |
There was a problem hiding this comment.
Being more explicit about what canceling does when the dialog is shown because no proposal is selected.
| }} | ||
| > | ||
| <Modal.Header closeButton> | ||
| <Modal.Title>Select a session</Modal.Title> |
There was a problem hiding this comment.
There seems to be a bit of confusion in the UI with the terms "proposal" and "session". Are those the same thing?
There was a problem hiding this comment.
:), almost but not really. When using proposal to login they are considered the same but when using user based login they are not.
There was a problem hiding this comment.
You are right that it is confusing, the form should really be called SelectSession or similar. This can be changed in a future PR.
| <th>Portal</th> | ||
| <th>User</th> | ||
| <th>Logbook</th> | ||
| <th scope="col">ID</th> |
There was a problem hiding this comment.
This heading cell was missing.
| </del> | ||
| ); | ||
| if (sessions.length === 0) { | ||
| return <span className={styles.fallback}>No sessions</span>; |
There was a problem hiding this comment.
Showing a fallback instead of an empty table.
| onKeyDown={(evt) => { | ||
| // For keyboard accessibility; ideally add "Select" button to each row instead of making rows clickable | ||
| if (evt.key === 'Enter' || evt.key === ' ') { | ||
| onSessionSelected(session); | ||
| } | ||
| }} |
There was a problem hiding this comment.
I'm including a bunch of accessibility fixes like this one and various aria-label attributes.
| @@ -105,7 +107,7 @@ class ServerIO { | |||
| this.hwrSocket.on('disconnect', (reason) => { | |||
| console.log('hwrSocket disconnected!'); // eslint-disable-line no-console | |||
|
|
|||
| setTimeout(() => { | |||
| this.connectionLostTimeout = setTimeout(() => { | |||
There was a problem hiding this comment.
This timeout was triggered sometimes when logging out and back in quickly, which made the connection-lost dialog flash on the screen.
There was a problem hiding this comment.
Well spotted, nice fix !
|
Thanks, 👍 |
| getLoginInfo(), | ||
| ), | ||
| ); | ||
|
|
This is based on
mo-limsand meant to be merged into it.