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

Use input widget state to style task inputs #5091

Merged
merged 13 commits into from Nov 27, 2018
Merged

Use input widget state to style task inputs #5091

merged 13 commits into from Nov 27, 2018

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Nov 26, 2018

Use checkbox/radio button state (active, checked or focussed) to control the style of adjacent task labels.
Remove onFocus(), onBlur(), unfocus() and associated tests from TaskInputField.
Convert TaskInputField to a function and add React.memo() for speed.
Works around some problems with the multiple choice and single answer task tests by rewriting them to use shallow rendering. Full mounting of those components won't work until enzyme supports React.memo().
Fixes #5096.

This PR should fix slow/laggy behaviour on phones, where there could be a noticeable delay between tapping an answer and seeing it change colour.

Staging branch URL: https://pr-5091.pfe-preview.zooniverse.org

Required Manual Testing

  • Does the non-logged in home page render correctly?
  • Does the logged in home page render correctly?
  • Does the projects page render correctly?
  • Can you load project home pages?
  • Can you load the classification page?
  • Can you submit a classification?
  • Does talk load correctly?
  • Can you post a talk comment?

Review Checklist

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you rm -rf node_modules/ && npm install and app works as expected?
  • If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
  • Are the tests passing locally and on Travis?

Optional

@coveralls
Copy link

coveralls commented Nov 26, 2018

Coverage Status

Coverage decreased (-0.2%) to 48.026% when pulling 7132bb3 on task-input-styles into 74a3e43 on master.

@eatyourgreens
Copy link
Contributor Author

Tests are broken because of enzymejs/enzyme#1875. The multiple choice and single answer question tests both run a full mount of the task component, then run tests on the input field children.

Use checkbox/radio button state (active, checked or focussed) to control the style of adjacent task labels.
Remove onFocus(), onBlur(), unfocus() and associated tests from TaskInputField.
Remove a ref that's no longer used, and a label prop that didn't seem to be referenced anywhere.
Move classname up to the container label.
Convert task input field to a functional component and add React.memo to speed up rendering.
Remove onChange() method and update tests.
Rewrite tests to use shallow rendering and test props of the generic task component, not nested children.
Rewrite tests to use shallow rendering and test props of the generic task component, not nested children.
Remove shouldInputBeChecked and move responsibility for checked logic up to parent task. Add boolean checked prop.
Use defaultChecked instead of checked, so that checkbox state controls the annotation, rather than vice-versa.
Remove shouldInputBeAutoFocussed. Add boolean autoFocus prop instead, and move autofocus logic up to parent tasks.
Use the as prop to changed the rendered HTML element.
Apply hover styles directly to task input labels.
Restore styles for active state (input:checked), input checked and focussed, and label hover on checked imputs.
Move common hover styles into a shared object.
Define constants for the default, hover and checked themed styles.
Use the BDD syntax from sinon-chai.
@eatyourgreens
Copy link
Contributor Author

Single answer and multiple choice answers seem to be faster in the dev classifier on this branch, but drawing tool radio buttons are still really slow. I'm not sure why. 😞

@srallen srallen self-assigned this Nov 27, 2018
Copy link
Contributor

@srallen srallen left a comment

Choose a reason for hiding this comment

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

LGTM. I hadn't refactored this yet in the rewrite code, but had left a note to do so. I'll make sure this gets done there.

For the highlighter tool, I noticed that the radio inputs aren't getting the checked styling, but they also aren't on the master branch. I wasn't sure if you were going to fix this in this branch or not. Do you know if any projects are using this tool type in production? I'm guessing not and if not, then I'm ok with that merging as is.

Not sure why the drawing task inputs are still slow, either.

@eatyourgreens
Copy link
Contributor Author

Thanks for taking a look. The highlighter options are buttons that you press after each text selection (like the bold/italic buttons in a word processor), so they don’t have a checked state. I’m happy to look at those in a different PR, if these changes are ok.

Copy link
Contributor

@srallen srallen left a comment

Choose a reason for hiding this comment

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

You know what, I think the highlighter tool confused me in the same way last time I looked at this. Since we're reusing the styling for the radio button inputs, I think the highlighter tool is the same and I try to click on the color I want first then go highlight, but it doesn't work that way. Maybe @beckyrother has a suggestion on how these buttons should be styled because I doubt I'm the only one who would get confused by this. Otherwise this is fine by me.

@srallen srallen merged commit f2ecaf2 into master Nov 27, 2018
@srallen srallen deleted the task-input-styles branch November 27, 2018 21:23
@beckyrother
Copy link
Contributor

beckyrother commented Nov 27, 2018

Is there a project that uses this to take a look at?
Maybe the highlight button is inactive until text is selected, with a rollover or modal on click that explains that text needs to be selected first before this can be used.

@srallen
Copy link
Contributor

srallen commented Nov 27, 2018

@beckyrother I think @eatyourgreens or @simensta know which projects that use this. You can also see a contrived example in the dev classifier: https://master.pfe-preview.zooniverse.org/dev/classifier select "Annotate Text" as well as the last frame for the subject viewer to get a text subject.

@beckyrother
Copy link
Contributor

Thanks Sarah! You're right, this is confusing, especially since the drawing task works the opposite way – clicking the type of annotation first and then clicking in the image. Is this possible with text? That is, click the highlight tool first and then select the text?

@srallen
Copy link
Contributor

srallen commented Nov 27, 2018

@beckyrother I've opened #5099 so we can keep track in an issue and have copied your question over to there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text highlighter button labels are unstyled
4 participants