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

OC: Timestamp and avatar grouping (Jul 1) #2939

Closed
compulim opened this issue Feb 24, 2020 · 1 comment · Fixed by #3365 or #3415
Closed

OC: Timestamp and avatar grouping (Jul 1) #2939

compulim opened this issue Feb 24, 2020 · 1 comment · Fixed by #3365 or #3415
Assignees

Comments

@compulim
Copy link
Contributor

compulim commented Feb 24, 2020

Feature Request

Background of this work can be found in BasicTranscript.js.

Currently, timestamp grouping is based on nextVisibleActivity (the activity rendered below the current one). For example, if the next visible activity fall into the same timestamp group (e.g. within 5 minutes of each other), only one timestamp will be shown.

Avatar is more-or-less the same, but it requires prevVisibleActivity (the activity rendered above).

Today, the activity renderer (or middleware) is also responsible for rendering timestamp and avatar. The function call is like renderActivity(): false | React.Element.

That means, when we call renderActivity, it either return false if the activity should be hidden (e.g. event activity), or, it immediate render as React.Element.

Since renderer is responsible for rendering timestamp and avatar, when we call this function, the renderer need to know nextVisibleActivity (note the word "visible", because renderer can hide activity by returning false).

Today, we do it by calling renderActivity in a reverse way (render most recent activity first). This will help us to construct the nextVisibleActivity. But it is technically impossible to construct prevVisibleActivity.

In order to group both timestamp and avatar, we need to update the activity renderer signature to renderActivity(): false | () => React.Element.

That means, the renderer can decide if they want to hide, or return a function which will definitely rendering an activity.

In this way, we can easily do a two-pass rendering and pass nextVisibleActivity and prevVisibleActivity to activity renderer. Thus, activity renderer can group both timestamp and avatar.

However, signature change is not trivial. We need some designs and possibly trade-offs (e.g. if devs are not using the new signature, avatar grouping will be disabled, plus warning on deprecations).

The new signature will be:

renderWebChat({
  activityMiddleware: () => next => ({ activity }) => {
    if (activity.type !== 'message') {
      return false;
    }

    return ({ nextVisibleActivity, prevVisibleActivity }) => (
      <div>...</div>
    );
  }
});

Comparing to the old signature:

renderWebChat({
  activityMiddleware: () => next => ({ activity, nextVisibleActivity }) => {
    if (activity.type !== 'message') {
      return false;
    }

    return (
      <div>...</div>
    );
  }
});

Additional Context

Introduce <ActivityContainer>, a common container

While working on this feature, we can consider creating a <ActivityContainer> component, which handle both avatar and timestamp. The user of <ActivityContainer> will only need to take care about the actual content.

This may significantly simplify the common code in <CarouselLayout> and <StackedLayout>, especially given RTL + bubble nub already added quite some complexity.

Also, we could easily allow developers to move timestamp on top of the bubble.

We should also expose this component for developers to use.

Converting all renderers

To reduce confusions, we should also consider converting all renderers to use this new signature.

Consolidating <BasicTranscript> and <BasicTranscriptContent>

As we added "enable composition mode" feature, the <BasicTranscriptContent> is pretty empty and we should consolidate it back.

[Enhancement]

@compulim compulim added backlog Out of scope for the current iteration but it will be evaluated in a future release. and removed Pending labels Mar 31, 2020
@compulim compulim removed their assignment Apr 2, 2020
@compulim
Copy link
Contributor Author

compulim commented Apr 2, 2020

Moving out from R9. This is not required until October release.

@compulim compulim added the R10 label Jun 3, 2020
@compulim compulim changed the title Timestamp and avatar grouping IC3: Timestamp and avatar grouping (Jul 1) Jun 12, 2020
@compulim compulim assigned compulim and unassigned compulim Jun 12, 2020
@compulim compulim self-assigned this Jun 24, 2020
@compulim compulim changed the title IC3: Timestamp and avatar grouping (Jul 1) OC: Timestamp and avatar grouping (Jul 1) Jul 8, 2020
@cwhitten cwhitten removed the backlog Out of scope for the current iteration but it will be evaluated in a future release. label Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants