QuickTalk: Refactor to functional component #3213

shaunanoordin · 2022-05-26T00:02:14Z

PR Overview

Package: lib-classifier
Affects: experimental QuickTalk feature (only affects projects with experimental tool enabled)

This PR refactors the QuickTalkContainer into a functional component.

No actual functions or features have been changed (EDIT: except for the two minor changes below). Users should see no difference in the feature.
Minor: error messages now use translated text.
Minor: when QuickTalk panel opens, close button now automatically gets focus. (I thought I added this in 3048, but I must have dropped it, d'oh!)

Testing

Test on lib-classifier (not app-project) with a project with QuickTalk enabled, e.g. https://local.zooniverse.org:8080/?env=staging&project=darkeshard%2Ftest-project-2022&workflow=3581
You should be able to see the QuickTalk button on the lower-right of the screen, with a number if any comments already exist.
If you click on the button, the QuickTalk panel should expand, and you should see any comments already made.
If you're logged in, you should be able to post comments on the expanded panel.

Known UI issues: (collapsed) QuickTalk button should have a background so it doesn't blend into the page (z-index madness) ; a message saying "you should be logged in if you want to post a message" should appear on the expanded panel.

Status

Ready for review! 👍

coveralls · 2022-05-26T00:16:51Z

Coverage increased (+0.02%) to 83.283% when pulling c7dec75 on fix-quicktalk-auth-mk2 into ad72d01 on master.

eatyourgreens · 2022-05-26T02:15:22Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

@@ -174,21 +148,21 @@ class QuickTalkContainer extends React.Component {

      const authorization = await getBearerToken(authClient)  // Check bearer token to ensure session hasn't timed out


I think I’ve pointed this bug out before but this will return an empty string if you call it before you call checkCurrent. See #2986.

eatyourgreens · 2022-05-26T02:19:24Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

@@ -198,11 +172,10 @@ class QuickTalkContainer extends React.Component {
        }

        await talkClient.type('comments').create(comment).save()


This could use panoptes.post to post comments with the new API client.

eatyourgreens · 2022-05-26T02:20:38Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

        author_ids = author_ids.filter((id, i) => author_ids.indexOf(id) === i)
-        
+
        apiClient.type('users').get({ id: author_ids })


This should use panoptes.get in the new API client.

eatyourgreens · 2022-05-26T02:22:27Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

          })
-        
+
        talkClient.type('roles')


Are Talk roles different from Panoptes roles? You can use the new client to get roles. The useProjectRoles hook has an example.

eatyourgreens · 2022-05-26T02:24:02Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

@@ -101,24 +86,24 @@ class QuickTalkContainer extends React.Component {
      page: 1,
      sort: 'created_at',  // PFE used '-created_at' to sort in reverse order, and I have no idea why.
    }
-
+
    talkClient.type('comments').get(query)


The new API client could be used here too.

eatyourgreens · 2022-05-26T02:36:13Z

There’s an auth bug in the existing code, which can be fixed here.

All the API calls should use the new panoptes client but maybe do that in a separate PR? You shouldn’t need to import any code from the old panoptes-client and you aren’t doing anything here that isn’t being done with the new client elsewhere in the classifier.

eatyourgreens

Looks good but:

there's an auth bug which blocks the first posted comment from being posted.
all API requests should really be handled with panoptes-js not the old client.
after I post a comment, keyboard focus goes back to the window, or parent document. I'm not sure. Either way, posting a comment from the keyboard works until after the comment is posted, at which point the Quick Talk panel loses keyboard focus.

eatyourgreens · 2022-05-26T10:52:01Z

This might be useful for managing keyboard focus.

eatyourgreens · 2022-05-26T15:14:46Z

packages/lib-classifier/src/components/Classifier/components/QuickTalk/QuickTalkContainer.js

@@ -174,21 +148,21 @@ class QuickTalkContainer extends React.Component {

      const authorization = await getBearerToken(authClient)  // Check bearer token to ensure session hasn't timed out
      const user = await authClient.checkCurrent()


Another thought about auth: Panoptes user sessions are quite long-lived, so call this once, when the component mounts, in order to establish a session (if one exists.)

Then get a bearer token as and when you need access to perform a restricted action, like posting a comment.

You can also use the usePanoptesUser and usePanoptesAuth hooks, although that would then limit you to following the Rules of Hooks eg. no conditional execution.
https://github.com/zooniverse/front-end-monorepo/tree/master/packages/lib-classifier/src/hooks#usepanoptesauth

shaunanoordin · 2022-05-27T14:40:37Z

Thanks Jim! 👍 I'm merging this PR, with the following goals for the follow up PR:

"First comment not posting" auth bug to be investigated
apiClient and talkClient to be changed to use new panoptes-js client.
(Address UI visual + accessibility improvements? Might put that in a third PR)

eatyourgreens · 2022-05-27T14:44:03Z

This is the auth bug.
#3213 (comment)

shaunanoordin requested a review from eatyourgreens May 26, 2022 00:02

eatyourgreens self-assigned this May 26, 2022

eatyourgreens reviewed May 26, 2022

View reviewed changes

eatyourgreens force-pushed the fix-quicktalk-auth-mk2 branch from de64631 to 41649bf Compare May 26, 2022 02:32

eatyourgreens approved these changes May 26, 2022

View reviewed changes

github-actions bot added the approved This PR is approved for merging label May 26, 2022

eatyourgreens reviewed May 26, 2022

View reviewed changes

shaunanoordin added 5 commits May 27, 2022 14:27

WIP convert QuickTalkContainer to functional

229cfd5

WIP QuickTalkContainer.fetchComments() now functional

f0b51fa

Change QuickTalkContainer to a functional component

7915f38

Change QuickTalk text to translated versions

9b1d94a

QuickTalk: auto focus on close button when panel opens

c7dec75

shaunanoordin force-pushed the fix-quicktalk-auth-mk2 branch from 41649bf to c7dec75 Compare May 27, 2022 13:27

shaunanoordin merged commit 31c4cf9 into master May 27, 2022

shaunanoordin deleted the fix-quicktalk-auth-mk2 branch May 27, 2022 14:40

shaunanoordin mentioned this pull request Jun 1, 2022

QuickTalk: replace old Panoptes client with talkAPI #3244

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QuickTalk: Refactor to functional component #3213

QuickTalk: Refactor to functional component #3213

shaunanoordin commented May 26, 2022

coveralls commented May 26, 2022 •

edited

Loading

eatyourgreens May 26, 2022

eatyourgreens May 26, 2022

eatyourgreens May 26, 2022

eatyourgreens May 26, 2022

eatyourgreens May 26, 2022

eatyourgreens commented May 26, 2022

eatyourgreens left a comment

eatyourgreens commented May 26, 2022

eatyourgreens May 26, 2022

eatyourgreens May 26, 2022

shaunanoordin commented May 27, 2022

eatyourgreens commented May 27, 2022

		@@ -174,21 +148,21 @@ class QuickTalkContainer extends React.Component {

		const authorization = await getBearerToken(authClient) // Check bearer token to ensure session hasn't timed out

		@@ -198,11 +172,10 @@ class QuickTalkContainer extends React.Component {
		}

		await talkClient.type('comments').create(comment).save()

		author_ids = author_ids.filter((id, i) => author_ids.indexOf(id) === i)


		apiClient.type('users').get({ id: author_ids })

		@@ -174,21 +148,21 @@ class QuickTalkContainer extends React.Component {

		const authorization = await getBearerToken(authClient) // Check bearer token to ensure session hasn't timed out
		const user = await authClient.checkCurrent()

QuickTalk: Refactor to functional component #3213

QuickTalk: Refactor to functional component #3213

Conversation

shaunanoordin commented May 26, 2022

PR Overview

Testing

Status

coveralls commented May 26, 2022 • edited Loading

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens commented May 26, 2022

eatyourgreens left a comment

Choose a reason for hiding this comment

eatyourgreens commented May 26, 2022

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

eatyourgreens May 26, 2022

Choose a reason for hiding this comment

shaunanoordin commented May 27, 2022

eatyourgreens commented May 27, 2022

coveralls commented May 26, 2022 •

edited

Loading