-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Activity grouping feature #3365
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
Conversation
0305f7a to
4bda6d8
Compare
beyackle
left a comment
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.
Some legibility issues, but overall I like how this looks.
| // ) => React.Element | ||
|
|
||
| return function renderCarouselLayout(renderAttachment, props) { | ||
| typeof props === '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.
see below
|
Thanks for your review. Still prefer As many JS developers are familiar with these shortcuts and truthy/falsy values, I don't see readability as a big concern here. |
beyackle
left a comment
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.
Thanks for clearing up the motivation behind the syntax that, to my eyes, still looks strange. I trust your specs, though, and the fixes you made look good.
Still in draft mode, running tests on Azure DevOps.
Changelog Entry
Description
useCreateActivityRendererwill replaceuseRenderActivityuseCreateActivityStatsuRendererwill replaceuseRenderActivityStatus40pxNew
createRenderActivityhookThe old signature is:
The new signature is:
New
createRenderActivityStatushookThe old signature is:
The new signature is:
Specific Changes
Screenshot changes
There are 5 types of screenshot changes:
transcript.*.html,activityGrouping.*.htmlhooks.useCreate*.html,hooks.useRender*.htmlAdditional context
How it works
Font anti-alias
Updating many screenshots because of anti-alias. Cause is unknown but usually caused by bumping versions of Docker + Chrome.
Pixel-perfect carousel layout
Green layer is the old stacked layout (also same as the new carousel layout).
Red layer is the old carousel layout, which is a 1-2px offset from the old stacked layout.
We updated the carousel layout so it will align with the stacked layout for consistency. We chose stacked layout because it is the default layout.