-
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
lib-classifier: Refactor survey task Choices and ChoiceButton styling #6494
base: lib-classifier_survey-task-design-changes
Are you sure you want to change the base?
lib-classifier: Refactor survey task Choices and ChoiceButton styling #6494
Conversation
83fbe41
to
e6d08b0
Compare
…ove no longer applicable comments
e6d08b0
to
dd756b6
Compare
...ins/tasks/survey/components/components/Chooser/components/Choices/components/ChoiceButton.js
Outdated
Show resolved
Hide resolved
We discussed content overflow and how we can't always account for the edge case of a very long choice name. This may still happen. But what we can enforce, although more work on our end, is research teams putting spaces between their 'Other' categories so we can display proper line breaks. insect/bug, reptile/amphibian > insect / bug, reptile / amphibian, etc. |
Refactored from I'm not sure if the Choice component should be refactored as a Here are some related MDN links: |
…ssifier_chooser-styling-updates
…ssifier_chooser-styling-updates
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.
A couple of questions on first code read:
-
Can you point me to the Figma or UI description for how many columns should appear on phone-sized screens. I vaguely remember the mention of mobile always having two columns, but on this branch a screen width < 600px knocks the column count down to 1 with lots of whitespace on each side. That introduces so much scrolling for a survey task user.
-
Could you also point me to Figma or design decisions / examples for projects with "Hide all" selected in the lab's thumbnail config. The example projects and Figma resources linked in this PR all have thumbnails.
(Edit: I see one screenshot of hidden thumbnails in Survey Task: Choices and ChoiceButton styling updates #6238 I'm going to ask Sean about it there)
@@ -98,6 +143,7 @@ export function Choices ({ | |||
<StyledGrid | |||
role='menu' | |||
rowsCount={rowsCount} | |||
columnsCount={columnsCount} |
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.
rowsCount
and columnsCount
should be passed with a $
to suppress the console warning about passing invalid props to a DOM element. See an example in the styled-components v6 upgrade here and styled-components transient props docs.
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 added the 'no thumbnail' options in Figma. They were previously buried in the exploration section.
This is due to the 2 column layout request of
The two columns (500px) plus margins (horizontal, task area ~20px, page ~20px, total ~80px) force one column around less than 600px. But agreed, a lot of whitespace, maybe columns could use clamp at smaller screen sizes or something similar? |
Perhaps they could have their own rules just for mobile. As long as the columns are even, I don't think we'll encounter an instance when they're too narrow to display properly at 2 columns. |
...ins/tasks/survey/components/components/Chooser/components/Choices/components/ChoiceButton.js
Outdated
Show resolved
Hide resolved
...ins/tasks/survey/components/components/Chooser/components/Choices/components/ChoiceButton.js
Show resolved
Hide resolved
...ins/tasks/survey/components/components/Chooser/components/Choices/components/DeleteButton.js
Show resolved
Hide resolved
When I run this branch with lib-classifier locally and look at your workflow without thumbnails (https://local.zooniverse.org:8080/?project=1634&workflow=3763), the buttons are still 60px height. Do you still plan to reduce those to 40px? |
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 bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
New Feature
Refactoring
Post-merge