Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Classifier: add a loading state #2986

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Mar 23, 2022

  • Add a loading state to the classifier container. Defer rendering the Classifier until after the container is mounted and the store has initialised.
  • Fix a small auth bug in the UPP store. Get an access token after checking the current user session, before fetching user preferences.

#2863 re-opened #2954 on staging, because the classifier store now loads synchronously (loading in ClassifierContainer is never false.) This PR should fix that by deferring the first render of the Classifier until after the classifier store is initialised (and the classifier UPP store has been reset.) To test it out, load the classifier while logged out, then log in on the Classify page of a project so that you are signing in to an existing classifier store (with UPP loading state already set.)

Package:
lib-classifier

Closes #2954.

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

@eatyourgreens eatyourgreens added the bug Something isn't working label Mar 23, 2022
@@ -56,11 +56,10 @@ const UserProjectPreferencesStore = types
const { authClient } = getRoot(self)

try {
const authorization = yield getBearerToken(authClient)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this can be reliably reproduced, but I was seeing this occasionally return an empty string, even when I was logged in.

- Add a `loading` state to the classifier container. Defer rendering the `Classifier` until after the container is mounted and the store has initialised.
- Fix a small auth bug in the UPP store. Get an access token after checking the current user session, before fetching user preferences.
@eatyourgreens eatyourgreens force-pushed the classifier-loading-state branch from 4a26863 to 675e276 Compare March 23, 2022 11:05
@eatyourgreens
Copy link
Contributor Author

This is a quick fix for the loading state bug. #2940 adds classifier hooks to fetch user data, and that might also be a better long-term solution for fetching project preferences.

Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Looks good! When I navigate to a project where I've already seen the tutorial such as TESS, sign-in, and the tutorial modal does not appear. When I visit a project I have not seen before with my user account, the tutorial modal appears.

Additional testing of a random project like i-fancy-cats:

  • I can sign-in and sign-out
  • Make a classification
  • Load all FEM project pages

@eatyourgreens eatyourgreens merged commit 97f9207 into master Mar 23, 2022
@eatyourgreens eatyourgreens deleted the classifier-loading-state branch March 23, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Classifier: previously seen tutorial opens after logging in.
2 participants