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

docs(Prototype): Chat messages with reactions popover prototype#524

Merged
sophieH29 merged 36 commits intomasterfrom
docs/chat-messages-popover-prototype
Dec 6, 2018
Merged

docs(Prototype): Chat messages with reactions popover prototype#524
sophieH29 merged 36 commits intomasterfrom
docs/chat-messages-popover-prototype

Conversation

@sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Nov 26, 2018

ezgif com-crop

@sophieH29 sophieH29 self-assigned this Nov 26, 2018
@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Nov 26, 2018

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Nov 26, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@b355abd). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #524   +/-   ##
=========================================
  Coverage          ?   88.17%           
=========================================
  Files             ?       42           
  Lines             ?     1455           
  Branches          ?      212           
=========================================
  Hits              ?     1283           
  Misses            ?      167           
  Partials          ?        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b355abd...a77319d. Read the comment docs.

layershifter
layershifter reviewed Nov 27, 2018
View reviewed changes
@layershifter layershifter added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 27, 2018
bmdalex
bmdalex suggested changes Nov 27, 2018
View reviewed changes
bmdalex
bmdalex approved these changes Dec 4, 2018
View reviewed changes
Copy link
Collaborator

@bmdalex bmdalex 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 addressing

mnajdova
mnajdova reviewed Dec 5, 2018
View reviewed changes
mnajdova
mnajdova reviewed Dec 5, 2018
View reviewed changes
docs/src/prototypes/chatMessageWithPopover/Popover.tsx Outdated
this.state.popupOpened && this.setState({ popupOpened: false })
}

menuStyles = ({ theme: { siteVariables } }) => ({
Copy link
Contributor

@mnajdova mnajdova Dec 5, 2018

Choose a reason for hiding this comment

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

In my opinion all styles shuold be separate in different file then the definition of the component using them.

Copy link
Contributor Author

@sophieH29 sophieH29 Dec 5, 2018

Choose a reason for hiding this comment

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

agree that this might be better when having styles separately so the code will look cleaner. But I would leave exactly these styles where they are, so it be more understandable when the focused state is changed it has direct affect in styles

mnajdova
mnajdova reviewed Dec 5, 2018
View reviewed changes
mnajdova
mnajdova reviewed Dec 5, 2018
View reviewed changes
docs/src/prototypes/chatMessageWithPopover/Popover.tsx Outdated
'aria-label': 'more options',
},
]}
renderItem={this.renderItemOrContextMenu}
Copy link
Contributor

@mnajdova mnajdova Dec 5, 2018

Choose a reason for hiding this comment

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

Watch out, we merge the PR for removing the renderXXX methods from the component's API. This should be refactored. For more info, please see the examples on the PR #519 and the Issue #355

Copy link
Contributor Author

@sophieH29 sophieH29 Dec 5, 2018

Choose a reason for hiding this comment

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

Yeah, I just found out it yesterday when Popup hasn't render. Will take a look today how to solve it

mnajdova
mnajdova reviewed Dec 5, 2018
View reviewed changes
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 see the comments provided.

mnajdova
mnajdova approved these changes Dec 6, 2018
View reviewed changes
Copy link
Contributor

@mnajdova mnajdova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am approving these changes, but before you merge, please take a look into one issue that I found (or at least I expected different behavior), while testing locally. This are the steps:

  • hover on the message (the popover is open)
  • hover on the popover (all 5 items were shown)
  • hover the three dots and click one of the items in the menu
    The menu closed but the popup is still shown with all five things opened, until you click somewhere outside of the it.
    Here is a gif showing this: https://i.imgur.com/u8PV7l3.gif

@sophieH29 sophieH29 merged commit 9522901 into master Dec 6, 2018
@sophieH29 sophieH29 deleted the docs/chat-messages-popover-prototype branch December 6, 2018 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@kuzhelov kuzhelov Awaiting requested review from kuzhelov

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

4 more reviewers

@layershifter layershifter layershifter left review comments

@mnajdova mnajdova mnajdova approved these changes

@bmdalex bmdalex bmdalex approved these changes

@jurokapsiar jurokapsiar jurokapsiar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sophieH29 sophieH29

Labels

🚀 ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sophieH29 @DustyTheBot @mnajdova @bmdalex @jurokapsiar @layershifter @alinais @mituron