Skip to content
This repository has been archived by the owner on May 27, 2022. It is now read-only.

Wal 123 common msg component removing all msgs #46

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

MuffinLightning
Copy link

Depends on WAL-59

@MuffinLightning MuffinLightning self-assigned this Feb 11, 2019
@mseaward mseaward changed the base branch from develop to WAL-59-port-bookie-common-message-component February 12, 2019 18:43
aametzler
aametzler previously approved these changes Feb 14, 2019
@mseaward mseaward changed the base branch from WAL-59-port-bookie-common-message-component to develop February 15, 2019 18:40
mseaward
mseaward previously approved these changes Feb 21, 2019
@aametzler aametzler dismissed stale reviews from mseaward and themself via 77a15e3 February 26, 2019 15:29
@aametzler aametzler requested a review from mseaward February 26, 2019 15:45
@aametzler aametzler self-assigned this Feb 26, 2019
src/reducers/CommonMessageReducer.js Outdated Show resolved Hide resolved
src/components/CommonMessage/CommonMessage.jsx Outdated Show resolved Hide resolved
src/components/CommonMessage/CommonMessage.scss Outdated Show resolved Hide resolved
@aametzler aametzler requested a review from mseaward February 28, 2019 14:12
mseaward
mseaward previously approved these changes Mar 1, 2019
Modify classname assignment to support true BEM.
<p onClick={ () => props.clearMessage(id) }>X</p>
</span>
<div className='cmn-msg__cont'>
<span className='cmn-msg__cont-txt'>{pair.get('content') }</span>

Choose a reason for hiding this comment

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

if this is a modifier, shouldn't it be 'cmn-msg__cont--text'?

Copy link
Contributor

Choose a reason for hiding this comment

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

it isn't a modifier

</span>
<div className='cmn-msg__cont'>
<span className='cmn-msg__cont-txt'>{pair.get('content') }</span>
<p className='cmn-msg__cont-dismiss'onClick={ () => props.clearMessage(id) }>X</p>

Choose a reason for hiding this comment

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

if this is a modifier, shouldn't it be 'cmn-msg__cont--dismiss'?

Copy link
Contributor

Choose a reason for hiding this comment

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

it isn't a modifier

src/components/CommonMessage/CommonMessage.scss Outdated Show resolved Hide resolved
src/constants/Message.js Outdated Show resolved Hide resolved
// Use lodash for a deep comparison of the merged messages.
if (!_.isEqual(propsMerged, prevPropsMerged)) {
this.checkToAssignTimer(propsMerged);
if (!_.isEqual(this.props.headerMessages, prevProps.headerMessages)) {

Choose a reason for hiding this comment

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

Common Message no longer displays under the header. Looks like something broke in this commit.

@mseaward mseaward requested a review from aametzler March 6, 2019 14:02
aametzler
aametzler previously approved these changes Mar 6, 2019
@aametzler aametzler self-requested a review March 7, 2019 14:50
Copy link

@aametzler aametzler left a comment

Choose a reason for hiding this comment

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

good job

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

Successfully merging this pull request may close these issues.

3 participants