Skip to content
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

Activity grouping feature #3365

Merged
merged 100 commits into from
Jul 31, 2020
Merged

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Jul 27, 2020

Still in draft mode, running tests on Azure DevOps.

Fixes #2939

Changelog Entry

Description

  • Bubbles are now grouped in transcript with 2-level grouping
    • Group by sender, then group by status
    • Status = timestamp or "Send failed." prompt
  • Avatar grouping can be set at 3 different levels:
    • Avatar on activities sharing the same sender
    • Avatar on activities sharing the same status (default)
    • Avatar on every activity
  • Added 2 hooks to deprecate older hooks with a newer API which is more consistent and flexible
    • useCreateActivityRenderer will replace useRenderActivity
    • useCreateActivityStatsuRenderer will replace useRenderActivityStatus
  • Simplification of avatar options
    • Avatar will align top/bottom depends on nub position
    • Avatar gutter (space reserved for avatar) will be either fixed width, or zero (no avatar). Default width is 40px

New createRenderActivity hook

Tests are added to make sure: new hooks works with old middleware. The new middleware with old hooks.

The old signature is:

const renderActivity = useRenderActivity();
const element = renderActivity({ activity });

return <div>{element}</div>;

The new signature is:

const createActivityRenderer = useCreateActivityRenderer();
const renderActivity = createActivityRenderer({ activity }); // If renderer decided to skip this activity, it will return `false`
const element = renderActivity && renderActivity({ hideTimestamp: false }); // Render-time options

return <div>{element}</div>;

New createRenderActivityStatus hook

Tests are added to make sure: new hooks works with old middleware. The new middleware with old hooks.

The old signature is:

const element = useRenderActivityStatus({ activity });

return <div>{element}</div>;

The new signature is:

const createActivityStatusRenderer = useCreateActivityStatusRenderer();
const renderActivityStatus = createActivityStatusRenderer({ activity });
const element = renderActivityStatus && renderActivityStatus();

return <div>{element}</div>;

Specific Changes

Screenshot changes

There are 5 types of screenshot changes:

  1. New tests
    • Layout tests: transcript.*.html, activityGrouping.*.html
    • Hooks tests: hooks.useCreate*.html, hooks.useRender*.html
  2. (~80%) Font anti-aliasing issue.
  3. Avatar gutter now have fixed width.
  4. The newer set for carousel layout is now pixel-perfect with stacked layout.
    • The activity status in previous set was 1-2 pixels off vertically
  5. Side must either has avatar, or don't have avatar at all.
    • We no longer support hybrid (some bubbles on one side has avatar, some don't)
    • It is okay to have avatar on one side, and no avatar on another side

Additional context

How it works

This show avatar on activities sharing the same status.

image

Font anti-alias

Updating many screenshots because of anti-alias. Cause is unknown but usually caused by bumping versions of Docker + Chrome.

image

image

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.

image


  • Testing Added

@compulim compulim marked this pull request as draft July 27, 2020 20:20
@compulim compulim marked this pull request as ready for review July 27, 2020 21:53
@compulim compulim changed the title [DRAFT] Activity grouping feature Activity grouping feature Jul 28, 2020
Copy link
Contributor

@beyackle beyackle left a 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.

__tests__/html/__jest__/runPageProcessor.js Outdated Show resolved Hide resolved
__tests__/html/__jest__/sleep.js Outdated Show resolved Hide resolved
__tests__/html/assets/activityGrouping.js Show resolved Hide resolved
__tests__/html/assets/activityGrouping.js Show resolved Hide resolved
__tests__/html/assets/activityGrouping.js Show resolved Hide resolved
// ) => React.Element

return function renderCarouselLayout(renderAttachment, props) {
typeof props === 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below

packages/component/src/BasicTranscript.js Outdated Show resolved Hide resolved
packages/component/src/BasicTranscript.js Outdated Show resolved Hide resolved
@compulim
Copy link
Contributor Author

compulim commented Jul 30, 2020

Thanks for your review.

Still prefer &&, ||, and ~, over if, >= 0. As a web component, we are building block of complex apps, thus, saving bandwidth, reducing latencies, and improving performance, are our priorities.

As many JS developers are familiar with these shortcuts and truthy/falsy values, I don't see readability as a big concern here.

Copy link
Contributor

@beyackle beyackle left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OC: Timestamp and avatar grouping (Jul 1)
2 participants