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

fix modal-subtask buttons on touch devices #3568

Merged
merged 5 commits into from
Aug 25, 2022
Merged

fix modal-subtask buttons on touch devices #3568

merged 5 commits into from
Aug 25, 2022

Conversation

MaysaB
Copy link
Contributor

@MaysaB MaysaB commented Aug 25, 2022

Staging branch URL:
https://github.com/zooniverse/front-end-monorepo/tree/fix-3344

Package

lib-classifier
lib-react-components

Linked Issue and/or Talk Post

Fixes #3344

Changes

Problem - the subtask modal buttons on 'Save and Close' and 'X' do not work on mobile devices. Pointer actions outside of task area for pop up is being overidden and being replaced by drag actions.

Analysis - The 'Save and Close' button, as well as the 'Close' (x) button were missing the property that the task area has (className="subtaskpopup-element-that-ignores-drag-actions"). This property doesn't allow the pointer actions to be overridden.

Solution - add the property to both the Save and Close button in a element and also to the element so that they ignore the drag actions (but only on those elements.)

See screen recording below.
View on an iPhone12Pro 390x844 at 125%

Screen.Recording.2022-08-23.at.16.14.11.mov

How to Review

Helpful explanations that will make your reviewer happy:

  • Zooniverse Projects to review UX

This is production env:
(https://local.zooniverse.org:3000/projects/maceb/drawing-project/classify/workflow/3614?reload=0&workflow=3614)

This is staging env.
https://local.zooniverse.org:8080/?env=staging&project=maceb%2Fdrawing-project&workflow=3614

To view the solved bug:

  1. Go to the staging environment
  2. Create a new project, add a drawing tool task, add a subtask to the drawing tool task, ensure you've added a subject.
  3. Go to classify. Draw something on your subject. Click on the drawing to bring up the subtask.
  4. Go to developer tools and change the dimensions to AirPad or iPhone12.
  5. Close the subtask using the 'save and close'.
  6. Click the drawing again, close the subtask using the (x) on the top right hand corner of the component.

Ensure you have done yarn build on both lib-classifier and lib-react-components, then run yarn dev on app-project to run production env

To test on mobile, Safari v.15.6:

  1. Find your laptop’s IP address (on MacOS you can hold option + click on wifi symbol)
  2. Run app-project locally
  3. Connect your mobile device to the same wifi network as your laptop
  4. Visit the project url on your mobile device, for example: https://your.ip.address.here:3000/projects/brooke/i-fancy-cats or https:/your.ip.address.here:3000/projects/maceb/drawing-project/classify/workflow/3614?reload=0&workflow=3614

Which storybook stories should be reviewed?

This is lib-react components:

http://localhost:6006/?path=/story/components-movablemodal--sub-task&globals=locale:en;locales.en:English;locales.test:Test+Language

This is lib-classifier:

http://localhost:6007/?path=/story/drawing-tools-transcription-line--subtask&globals=locale:en;locales.en:English;locales.test:Test+Language

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

  • [ 822 tests are passing, 14 pending due to .skip ]
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox v.104, Chrome v.104, Safari v.15.6
  • FEM works in a mobile browser, Safari v.15.6.

General UX

Example Staging Project: i-fancy-cats
https://local.zooniverse.org:3000/projects/maceb/drawing-project/classify/workflow/3614?reload=0&workflow=3614

  • 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

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

New Feature

  • The PR creator has listed user actions to use when testing the new feature
  • Unit tests are included for the new feature
  • A storybook story has been created or updated
    • Example of SlideTutorial component with docgen comments, and its story

Refactoring

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

Maintenance

  • If not from dependabot, the PR creator has described the update (major, minor, or patch version, changelog)

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. I'm not sure why the tests are failing. There's nothing obvious in the changes that should break any tests. I think this can be merged once we've figured out why the tests failed.

…ubjectViewer/components/InteractionLayer/components/SubTaskPopup/SubTaskPopup.js

Co-authored-by: Jim O'Donnell <[email protected]>
@MaysaB
Copy link
Contributor Author

MaysaB commented Aug 25, 2022

Woopsie, thanks Jim. I've been trying to figure it out but my eyes burn now so i just submitted the pull request. Would love to know whats going on with it!

@eatyourgreens
Copy link
Contributor

Tests like this one have broken but this is a bad test anyway.

@eatyourgreens
Copy link
Contributor

@MaysaB #3570 should fix the tests. Let me know if you have any questions about it.

- add a `subtaskpopup-task` CSS class to task components.
- use that class to find tasks in the tests.
@coveralls
Copy link

coveralls commented Aug 25, 2022

Coverage Status

Coverage remained the same at 83.082% when pulling 2672a52 on fix-3344 into 6d83a31 on master.

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@github-actions github-actions bot added the approved This PR is approved for merging label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transcription Task: Subtask modal buttons do not work in mobile browser
3 participants