-
Notifications
You must be signed in to change notification settings - Fork 51
feat(chat): chat item gutter and refactoring #556
Conversation
28dffda to
67a4072
Compare
|
Some thoughts on the raised issues:
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.
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)
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): |
623826f to
b822e11
Compare
e7d2f9d to
1b39270
Compare
mnajdova
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.
Please take a look on the comments. Let's resolve/comment on them and push this forward.
1b39270 to
1dcbe35
Compare
aa8db9d to
2265835
Compare
Update on final changes based on latest sync with the team:
New API:<Chat.Item
gutterPosition='start'
gutter={{ content: <Avatar …/> }}
message={{ content: <Chat.Message …> }}
/> |
- addressed comments - imporoved styles
… using children css selectors
- improved examples; - added fix for gutter being created by default;
- replaced Chat.Item content prop with message; - deprecated Children API;
2265835 to
225c7e8
Compare
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.
Looks good, please just update the propTypes for the ChatMessage component (see #556 (comment)) You can merge after that.
feat(chat): chat item gutter and refactoring
Description
This PR implements solution 1 from #537 and closes #537
We need to refactor our
Chatsubcomponents,Chat.ItemandChat.Messagefor the following reasons:Chat.Messageis too specific (theavatarshould be more top level)Chat.Itemis not specific enough (needsgutter)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:
avatarslot fromChat.Messagegutterslot toChat.Item;gutterPosition='start' | 'end'prop toChat.Item;contentslot tomessageforChat.Item;Chatcomponents;API:
Known issues:
mineprop toChat.Itemas well for styling purposes and deciding ongutterpositioning; other suggestion was:gutterPosition: 'before' | 'after'(*LE: decidedgutterPosition: 'start' | 'end')gutterandcontentslots we will need to specify thecontentandgutterprops as objects withcontentprops instead of just elements after merging fix(factories): shorthand fix for applying props to react element #496 (*LE: we will use verbosecontentprops for now and look into RFC: Shorthand polymorphism usingkindproperty #512 to simplify that)chatItemStylesare 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)maxWidth: 80%forgutter+contentrequires 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