-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement Homepage 2.2 #543
Conversation
const timer = setTimeout(() => { | ||
setTabIndex(nextIndex); | ||
selectTab(PREVIEW_ORDER[nextIndex]); | ||
}, 5000); |
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 personally think, that 5 seconds is not enough for user to try interact with preview. Maybe double it? Try it for yourself @hobuobi @terryttsai
Also, @terryttsai maybe you know where to disable start animation of |
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.
Awesome work! So exciting to see this implemented. A few comments here and there:
Major/Breaking
- The current implementation does not work with fewer than four visualizations (e.g., on Pennsylvania's homepage, it tries to rotate to an imaginary fourth visualization and shows nothing for five seconds).
- I noticed a strange bug on the other pages where the legends for the bubble charts seemed to overlap with the actual visualization... not sure why, but not seeing the same thing in staging or production!
Design Nits
- Use the typical body font for the text below the header (for ND, "At the department of corrections in North Dakota, we believe that sharing information builds greater accountability between us and the communities we serve.")
- Font size of the carousel labels ("Prison," "Racial Disparities," etc) could be larger. 1.5rem seemed like a possible good size on desktop.
- Button:
- Font size of the button is too small; should be about the same size as the carousel labels (suggested 1.5rem).
- Should add a shadow to the button. (Design spec is #006C67 at 0.3 opacity; Y-offset of 10; blur of 20px. Can play around with it a little!)
- Ideal would be to have the button width flex based on its contents, rather than stay a static width.
- The margins seem inconsistent (wider) with the Narrative pages.
- Can we see what this would look like if we added the legends back?
This should cover almost everything I caught! Thanks again for the hard work.
There's a comment in
Not sure what you mean. Could you tell more specific? Other changes was made as requested. |
Awesome work! To wrap some of this up: RESPONSES TO THE ABOVE
DESIGN NITS
FUNCTIONALITY
CONTENTPlease copy exactly as written. US_ND
US_PA
US_ID (subject to change, but good placeholder)
US_TN (subject to change, but good placeholder)
|
Pull Request Test Coverage Report for Build 2302413894
💛 - Coveralls |
I don't think dynamically changing a width of an element depending on its content is possible, as there is no transition for content. The content of an element changes instantly and the width does not get a numerical value in this case - but adjusts. |
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.
Thank you for addressing all of the comments! I think this is all good from a product/design POV; will leave it to @terryttsai to wrap up code review.
CC @agronomic in case he wants to check out the new design/functionality. :)
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.
LGTM!
0630bd5
to
5e41dd9
Compare
Description of the change
Implementing Homepage 2.2
Type of change
Related issues
Closes #522
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: