Classifier: Move MetaTools into same grid area as subject viewer #4021

goplayoutside3 · 2022-12-17T00:01:36Z

Package

lib-classifier

Linked Issue and/or Talk Post

Includes #4019
~~Includes #4005~~

Describe your changes

This PR moves the MetaTools component into the same grid area as the SubjectViewer, which allows its buttons to always display directly below the subject image regardless of viewport aspect ratio. Screenshots below.

This PR also includes refactoring from class to functional components and RTL unit tests for Metadata, MetadataModal, and MetadataButton.

Previously, the MetaTools buttons displayed below the toolbar height:

The changes in this PR display them like this:

How to Review

Run yarn bootstrap with this branch and view any project with a subject image. Examples:
- i-fancy-cats
- TESS
- Poets & Lovers

I also added a story for the MetaTools container with a specific case for smaller subject viewer styling.

See corresponding ImageToolbar refactoring in #4008.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

Refactoring

The PR creator has described the reason for refactoring
The refactored component(s) continue to work as expected

coveralls · 2022-12-17T02:44:47Z

Coverage: 82.226% (-0.03%) from 82.254% when pulling 5cb7796 on metatools-refactor into 4d08e18 on master.

eatyourgreens · 2022-12-17T09:09:46Z

This is going to break MetaTools on desktop, where it is supposed to extend under the Field Guide button.

Maybe small screens need to define a new grid layout, not use DefaultLayout.

eatyourgreens · 2022-12-17T09:14:03Z

New layouts can be added to the classifier here:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/Layout/helpers/getLayout/getLayout.js

Lines 3 to 5 in d592efd

    
           const layoutsMap = { 
        
             default: DefaultLayout 
        
           }

See also Shaun’s ADR about layouts.
https://github.com/zooniverse/front-end-monorepo/blob/master/docs/arch/adr-32.md

goplayoutside3 · 2022-12-17T19:38:47Z

Noting that this change was initially requested by @seanmiller26 during development of custom video controls Classifier: Custom Video Controls #3684 (review). Further discussion is planned around layouts, so no rush to merge this PR while waiting for a final design decision.
This is going to break MetaTools on desktop, where it is supposed to extend under the Field Guide button.

On desktop, when I view SuperWASP Black Hole hunters on production and locally, the classify page looks the same. When you say "break", do you mean the styling of MetaTools will respond differently to varying viewport widths?

eatyourgreens · 2022-12-19T08:59:53Z

Weird that the layout doesn’t change. The subject area is designed to match the height of the toolbar, so I assumed the SVG subject height would shrink to allow for the height of the toolbar in the subject area.

My concern was that this changes the height of the subject for live projects. If that’s not happening, then there’s no problem.

eatyourgreens · 2022-12-19T09:06:42Z

By the way, if you want to try out design ideas for new layouts, but don’t want to change PH-TESS etc., then adding a new layout to the layouts map would give you a layout that you can experiment with while also controlling which projects and workflows use it.

That might be useful for testing big changes, rather than editing the default layout directly.

mcbouslog · 2023-01-10T20:44:31Z

packages/lib-classifier/src/components/Classifier/components/MetaTools/MetaTools.js

+      <Metadata
+        isThereMetadata={isThereMetadata}
+        metadata={subject && subject.metadata}
+      />
+      <FavouritesButton
+        checked={subject?.favorite}
+        disabled={!subject || !upp}
+        onClick={subject?.toggleFavorite}
+      />
+      <CollectionsButton
+        disabled={!subject || !upp}
+        onClick={addToCollection}
+      />


When I run locally, signed in, and load https://local.zooniverse.org:3000/projects/brooke/i-fancy-cats/classify/workflow/693 or https://local.zooniverse.org:3000/projects/pmlogan/poets-and-lovers/classify/workflow/21362/subject-set/104809?env=production the Metadata, FavouritesButton, and CollectionsButton are unexpectedly disabled. Console logging the subject from MetaTools returns null and the component doesn't appear to rerender once the subject (and related isThereMetadata) is defined.

Oh I think the mobx observer should wrap the MetaTools export.
export default withResponsiveContext(observer(MetaTools)) with related mobx observer import.

You're right! I think adding observer restored some of the expected functionality, but I also noticed that you can visit i-fancy-cats on frontend.preview, sign-in to enable FavouritesButton and CollectionsButton, but then upon sign-out the upp from the store still contains user data, and the buttons are all still enabled. I think this is a bug of the UserProjectPreferences store so I'll do a quick search for an already existing Github Issue or open a new one.

mcbouslog

Looks good! Big improvement for small screens. Looked strange to me at first on desktop, but only because I'm used to current grid.

Refactors look good 👍 . Only blocking comment is regarding MetaTools missing observer (I think), otherwise a few other comments for consideration, but not blocking.

...er/components/MetaTools/components/Metadata/components/MetadataButton/MetadataButton.spec.js

packages/lib-classifier/src/components/Classifier/components/MetaTools/MetaTools.stories.js

mcbouslog · 2023-01-10T22:25:34Z

Unrelated to these changes, but while reviewing I noticed the Hide Previous Marks svg icon is 24px while the other MetaTools icons are 15px. The difference isn't noticeable (to me anyway) on desktop, but when stacked for a small screen the alignment is a little more noticeable. Not sure if want to include a fix for in these changes, we can convert this comment to an issue, or other option.

goplayoutside3 · 2023-01-11T22:54:13Z

...icon is 24px while the other MetaTools icons are 15px. The difference isn't noticeable (to me anyway) on desktop, but when stacked for a small screen the alignment is a little more noticeable.

I agree the difference is annoying when all meta buttons are stacked on top of each other when its column is of smaller width. I plan to follow-up with Sean to get his approval on this PR as well, and my review with him will include minor adjustments of icon size, margins, etc

mcbouslog

Changes LGTM 👍

goplayoutside3 · 2023-01-16T19:27:36Z

Met with @seanmiller26 to review MetaTools design. A few notes on what will follow in a separate PR:

Align all icons of meta tools buttons if stacked on top of each other
Work toward minor refactor of meta tools button responsive design (will open an Issue with design reference)
Make the MultiFrameViewer's Filmstrip sticky like the image toolbar

goplayoutside3 and others added 11 commits December 12, 2022 16:09

move resize observer from flipbook controls to DefaultLayout

3fba336

add SubjectViewerFactory and update DefaultLayout specs

6c9ebb6

Merge branch 'master' into viewer-grid-resize-observer

8b2cc1f

Merge branch 'master' into viewer-grid-resize-observer

2e87741

refactor MetaTools into functional component

5f52654

Merge branch 'viewer-grid-resize-observer'

c8fcf26

put MetaTools in same grid area as SubjectViewer

e372f92

add stories for MetaTools container

bd47256

refactpr Metadata to a functional component

b45fb2f

refactor Metadata's component's unit tests

b8f4974

remove +tab+ formatter

c5bee72

goplayoutside3 added refactor Refactoring existing code ui labels Dec 17, 2022

remove PR #4005

0c9fdc6

goplayoutside3 mentioned this pull request Dec 17, 2022

Classifier: Move resize observer into DefaultLayout to aid responsive styling #4005

Closed

11 tasks

Merge branch 'master'

16130da

mcbouslog self-assigned this Jan 9, 2023

mcbouslog reviewed Jan 10, 2023

View reviewed changes

mcbouslog requested changes Jan 10, 2023

View reviewed changes

...er/components/MetaTools/components/Metadata/components/MetadataButton/MetadataButton.spec.js Outdated Show resolved Hide resolved

packages/lib-classifier/src/components/Classifier/components/MetaTools/MetaTools.stories.js Outdated Show resolved Hide resolved

goplayoutside3 added 3 commits January 11, 2023 16:40

add observer to MetaTools

1b77ed7

update MetadataButton spec

38ed474

remove story's border

52b4794

goplayoutside3 requested a review from mcbouslog January 11, 2023 22:54

seanmiller26 self-requested a review January 12, 2023 16:01

mcbouslog approved these changes Jan 12, 2023

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Jan 12, 2023

Merge branch 'master'

71f450d

goplayoutside3 mentioned this pull request Jan 16, 2023

Meta Tools buttons responsive design #4140

Open

goplayoutside3 and others added 2 commits January 16, 2023 14:37

clean up

22274cd

Merge branch 'master' into metatools-refactor

5cb7796

goplayoutside3 merged commit b7b7ad0 into master Jan 16, 2023

goplayoutside3 deleted the metatools-refactor branch January 16, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classifier: Move MetaTools into same grid area as subject viewer #4021

Classifier: Move MetaTools into same grid area as subject viewer #4021

goplayoutside3 commented Dec 17, 2022 •

edited by mcbouslog

Loading

coveralls commented Dec 17, 2022 •

edited

Loading

eatyourgreens commented Dec 17, 2022

eatyourgreens commented Dec 17, 2022

goplayoutside3 commented Dec 17, 2022

eatyourgreens commented Dec 19, 2022

eatyourgreens commented Dec 19, 2022

mcbouslog Jan 10, 2023

mcbouslog Jan 10, 2023 •

edited

Loading

goplayoutside3 Jan 11, 2023

mcbouslog left a comment

mcbouslog commented Jan 10, 2023

goplayoutside3 commented Jan 11, 2023

mcbouslog left a comment

goplayoutside3 commented Jan 16, 2023 •

edited

Loading

Classifier: Move MetaTools into same grid area as subject viewer #4021

Classifier: Move MetaTools into same grid area as subject viewer #4021

Conversation

goplayoutside3 commented Dec 17, 2022 • edited by mcbouslog Loading

Package

Linked Issue and/or Talk Post

Describe your changes

How to Review

Checklist

General

General UX

Refactoring

coveralls commented Dec 17, 2022 • edited Loading

eatyourgreens commented Dec 17, 2022

eatyourgreens commented Dec 17, 2022

goplayoutside3 commented Dec 17, 2022

eatyourgreens commented Dec 19, 2022

eatyourgreens commented Dec 19, 2022

mcbouslog Jan 10, 2023

Choose a reason for hiding this comment

mcbouslog Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

goplayoutside3 Jan 11, 2023

Choose a reason for hiding this comment

mcbouslog left a comment

Choose a reason for hiding this comment

mcbouslog commented Jan 10, 2023

goplayoutside3 commented Jan 11, 2023

mcbouslog left a comment

Choose a reason for hiding this comment

goplayoutside3 commented Jan 16, 2023 • edited Loading

goplayoutside3 commented Dec 17, 2022 •

edited by mcbouslog

Loading

coveralls commented Dec 17, 2022 •

edited

Loading

mcbouslog Jan 10, 2023 •

edited

Loading

goplayoutside3 commented Jan 16, 2023 •

edited

Loading