-
Notifications
You must be signed in to change notification settings - Fork 30
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
refactor(drawing-tools): drag handle sizing #6685
refactor(drawing-tools): drag handle sizing #6685
Conversation
86c3933
to
5e8a416
Compare
|
||
const StyledCircle = styled('circle')` | ||
stroke-width: 2; | ||
&:hover { | ||
cursor: move; | ||
} | ||
` | ||
const RADIUS = screen.width > 900 ? 4 : 10 | ||
const RADIUS = screen.width > 900 ? 3 * STROKE_WIDTH : 5 * STROKE_WIDTH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be safer to make the drag handle size a multiple of the base stroke width here. If the stroke width changes, the default handle size will change proportionately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I thought maybe it should be 2 * STROKE_WIDTH
for the first condition (to match the previous calculation), but I think 3 is the right amount after reviewing visually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, 4px seems a bit small to me, so I bumped it to 6.
5e8a416
to
5ad0138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thank you @eatyourgreens !
Tested locally on projects linked and additional staging project with drawing tools.
Per your suggestion I had been trying non-scaling-size
, but was missing the related transform
that makes it work.
|
||
const StyledCircle = styled('circle')` | ||
stroke-width: 2; | ||
&:hover { | ||
cursor: move; | ||
} | ||
` | ||
const RADIUS = screen.width > 900 ? 4 : 10 | ||
const RADIUS = screen.width > 900 ? 3 * STROKE_WIDTH : 5 * STROKE_WIDTH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I thought maybe it should be 2 * STROKE_WIDTH
for the first condition (to match the previous calculation), but I think 3 is the right amount after reviewing visually.
- Remove `defaultProps` from functional components. - Refactor transcription lines to use translation and scale transforms for the line handles. - Style drag handles and point marks with `non-scaling-size`. - Fixes a small issue where drag handles aren't rendered in the correct proportion with respect to marks.
5ad0138
to
ef0ad05
Compare
|
||
return ( | ||
<g onPointerUp={active ? onFinish : undefined}> | ||
<g transform={`scale(${1 / scale})`} onPointerUp={active ? onFinish : undefined}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SVG spec says that non-scaling-size
ignores scale transforms, but points render much too large without scaling them down here. Could be a case of browsers not implementing the SVG spec, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These scale transforms may not be needed once the SVG zoom is handled with scale transforms, rather than cropping the viewbox.
By the way, CI is failing because coveralls.io had a major outage yesterday and they're still recovering. https://status.coveralls.io/ says use this workaround to avoid blocking CI pipelines:
|
The spec says that EDIT: according to the first comment here, only |
defaultProps
from functional components.non-scaling-size
.Here are screenshots of the sizing issue:
Main branch (drag handles are too small):

This branch (drag handles are correctly proportioned):

Test cases in the dev classifier:
Please request review from
@zooniverse/frontend
team or an individual member of that team.Package
lib-classifier
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
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Refactoring