Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(chat): chat item gutter and refactoring #556

Merged
merged 21 commits into from
Dec 13, 2018
Merged

Conversation

bmdalex
Copy link
Collaborator

@bmdalex bmdalex commented Dec 4, 2018

feat(chat): chat item gutter and refactoring

Description

This PR implements solution 1 from #537 and closes #537

We need to refactor our Chat subcomponents, Chat.Item and Chat.Message for the following reasons:

  • need better structuring and more flexibility;
  • need to solve specific clients needs when it comes to layout of components and providing very specific content to specific areas;
  • Chat.Message is too specific (the avatar should be more top level)
  • Chat.Item is not specific enough (needs gutter)

Solution (implemented in this PR)

This PR implements a mix of solution 1 from #537 and other discussion I had with the team; here are the main steps:

  • removing avatar slot from Chat.Message
  • adding gutter slot to Chat.Item;
  • adding gutterPosition='start' | 'end' prop to Chat.Item;
  • renaming content slot to message for Chat.Item;
  • deprecating Children API for Chat components;

API:

<Chat items={[
  {
    message: <AnyComponent .../>,
  },
  {
    mine: true,
    gutterPosition: 'end',
    gutter: { content: <Avatar image="" status="" /> },
    message: { content:
      <Chat.Message
        content="Let's have a call"
        author="John Doe"
        timestamp="Today, 11:15 PM"
        mine
      />,
    },
  },
]} />

Known issues:

  • need to add mine prop to Chat.Item as well for styling purposes and deciding on gutter positioning; other suggestion was: gutterPosition: 'before' | 'after' (*LE: decided gutterPosition: 'start' | 'end')
  • in order to leverage correct styles for gutter and content slots we will need to specify the content and gutter props as objects with content props instead of just elements after merging fix(factories): shorthand fix for applying props to react element #496 (*LE: we will use verbose content props for now and look into RFC: Shorthand polymorphism using kind property  #512 to simplify that)
  • chatItemStyles are too specific and are causing other components to be rendered incorrectly as content (Divider for instance, see below); we might have to go with solution 2 from Chat.Item and Chat.Mesasge refactor #537 and create subcomponents. (*LE: fixed by using new styles that do not rely on flexbox, by using css children selectors for styling)
    screenshot 2018-12-05 at 12 17 27
  • maxWidth: 80% for gutter + content requires complex styling in order to be achieved (*LE: fixed by using new styles that do not rely on flexbox, by using css children selectors for styling)

*LE: ALL ISSUES ARE HANDLED, GOOD FOR REVIEW

Legend:

*LE: later edit

@mnajdova
Copy link
Contributor

mnajdova commented Dec 5, 2018

Some thoughts on the raised issues:

need to add mine prop to Chat.Item as well for styling purposes and deciding on gutterpositioning; other suggestion was: gutterPosition: 'before' | 'after'

This was my only proposal on this, the positioning is something general that we have in multiple components, so it would be great if we are consistent about this.

in order to leverage correct styles for gutter and content slots we will need to specify the content and gutter props as objects with content props instead of just elements after merging #496

For this, I don't have smarter proposal (currently this is the only way we can make 'generic' slots. After implementing the 'kind' property, see issue: #512 we may provide different API here)

chatItemStyles are too specific and are causing other components to be rendered incorrectly as content (Divider for instance, see below); we might have to go with solution 2 from #537 and create subcomponents.

I would suggest to always use one display prop for component and try to arrange the items there dependently on the props. If we are using flex, then we should always stick with it. One simple example of how it can be refactored to use just flex is the following (dirty code alert - take this just as a example):

const chatItemStyles: ComponentSlotStylesInput<ChatItemProps, any> = {
  root: ({ props: p }): ICSSInJSStyle => ({
    display: 'flex',
    justifyContent: p.mine ? 'flex-end' :  p.gutter ? 'flex-start' : 'center',
    position: 'relative',
    marginTop: _14pxAsRem_,
    marginBottom: _14pxAsRem_,
  }),

  gutter: ({ props: p }): ICSSInJSStyle => ({
    flex: 'none',
    display: p.mine ? 'none' : undefined,
    marginTop: _10pxAsRem_,
    marginBottom: _10pxAsRem_,
    marginLeft: p.mine ? _10pxAsRem_ : 0,
    marginRight: p.mine ? 0 : _10pxAsRem_,
  }),
  content: ({props: p}): ICSSInJSStyle => ({
    ...(!p.gutter && !p.mine && {
      flex: 1,
    }),
  }),
}

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Please take a look on the comments. Let's resolve/comment on them and push this forward.

@bmdalex
Copy link
Collaborator Author

bmdalex commented Dec 12, 2018

Update on final changes based on latest sync with the team:

  • removed Chat.Gutter subcomponent because:
    • it doesn’t bring any value, it's just another component to support
    • it's not breaking any consistency or patterns as we have slots that do not have subcomponents (Button has icon slot but there is no Button.Icon)
    • we use Slot instead
  • renamed Chat.Item content prop to message
  • deprecated children API examples

New API:

<Chat.Item
  gutterPosition='start'
  gutter={{ content: <Avatar /> }}
  message={{ content: <Chat.Message > }}
/>

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good, please just update the propTypes for the ChatMessage component (see #556 (comment)) You can merge after that.

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

Successfully merging this pull request may close these issues.

Chat.Item and Chat.Mesasge refactor
4 participants