lib-classifier: Refactor CenteredLayout and survey task spacing #6615
Conversation
mcbouslog
changed the title
mcbouslog
changed the title
There was a problem hiding this comment.
The layouts are looking much better on this branch. One thing I noticed is that when CenteredLayout is used, the subject viewer does not behave in a sticky manner (regardless of the task). This is actually true on production too, so I'm wondering if you'd like to include a fix in this PR.
Here's some examples to work with:
Production example, CenteredLayout, no survey task: Daily Minor Planet
Production example, MaxWidthLayout, survey task: Offal Wildlife Watch
Local example, CenteredLayout, survey task: Woodpecker Cavity Cam
Local example, MaxWidthLayout, survey task: Your test project
Screen recorded example on this branch:
scroll.mov
packages/lib-classifier/src/plugins/tasks/survey/components/components/Chooser/Chooser.js
Outdated
Show resolved
Hide resolved
| if (props.$columnsCount === 3) return (props.$hideThumbnails ? '165.33px' : '166px'); | ||
| if (props.$columnsCount === 2) return (props.$hideThumbnails ? '248.5px' : '249px'); |
There was a problem hiding this comment.
I'm surprised to see fractions of a pixel here 🤔 Is it because you're using the column width to create a border around each Choice button?
There was a problem hiding this comment.
The grid gap(s) when there are no thumbnails causes the component to be wider than desired, the slightly smaller column widths here (when hiding thumbnails) keep the columns and component width as desired.
Co-authored-by: Delilah C. <[email protected]>
|
Yes nice catch regarding sticky subjects, I think I've fixed it in 947c6bf. |
|
Just pushed a commit to extract shared styled containers (Viewer, TaskArea, etc.) to a single file. It's a bit out of scope, but I think helpful, though also understand if want to exclude from these (survey task refactor) changes. Let me know if I should remove/revert it, no prob at all. |
| position: relative; | ||
| grid-gap: 1.875rem; | ||
| grid-template-areas: 'viewer task'; | ||
| grid-template-columns: auto ${props => (props.hasSurveyTask ? '33.75rem' : '25rem')}; |
There was a problem hiding this comment.
I think the grid-template-columns value and any following line (CenteredLayout -> justify-content: center, MaxWidth -> margin: auto, and NoMaxWidth nothing) are they key differences between layouts? Could add a related/similar comment to each layout accordingly if that would be helpful.
|
I started reviewing and agree it's a good idea to separate out the shared styled components for layouts. The Storybook for Layout is looking great. However, I moved on to running the classifier locally to test some production projects. 🚨 There are two unexpected behaviors to double check:
|
Woodpecker Cavity Cam: The Daily Minor Planet:
|
|
Could you double check all your latest changes are pushed to this branch? When I check it out and run locally I do not see the same layouts as in your screenshots. If needed, let's deploy this branch to fe-project-branch and Slack huddle to compare. For Spyfish Aotearoa does your question mark mean you ran master locally and do or do not see a task area loading successfully? |
Package
Linked Issue and/or Talk Post
Describe your changes
How to Review
Helpful explanations that will make your reviewer happy:
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 bootstrapordocker-compose up --buildand FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Bug Fix
Refactoring