Modals and DashAlert state #737

ackernaut · 2017-02-23T19:20:16Z

Fixes #724

mattkrick

well done! just 2 readability things & 1 slight performance thing & we'll be ready to rock

mattkrick · 2017-02-24T19:47:01Z

src/universal/components/Dashboard/DashModal.js

-  },
-
-  main: {
+  [ui.modalLayout[0]]: {


if we write the type out instead of referencing an array this would be easier to read

mattkrick · 2017-02-24T19:52:10Z

src/universal/modules/teamDashboard/components/Team/Team.js

  const {id: teamId, name: teamName, isPaid} = team;
  const hasActiveMeeting = Boolean(team && team.meetingId);
  const hasOverlay = hasActiveMeeting || !isPaid;
  const isSettings = router.isActive(`/team/${teamId}/settings`, false);
  initialValues.teamName = teamName;
  const DashHeaderInfoTitle = isSettings ? <EditTeamName initialValues={initialValues} teamName={teamName} teamId={teamId}/> : teamName;
+  const modalLayout = hasNotificationBar ? ui.modalLayout[1] : ui.modalLayout[0];


referencing the array isn't very meaningful (i know i set a bad example with the notifications varList, but that's mainly for payload size). could we instead say something like `hasNotificationBar ? ui.main : ui.mainHasNotificaitonBar

mattkrick · 2017-02-24T19:53:12Z

src/universal/containers/DashLayoutContainer/DashLayoutContainer.js

+    if (activeMeetings.length > 0 || barType === TRIAL_EXPIRES_SOON || barType === TRIAL_EXPIRED) {
+      this.props.dispatch(notificationBarPresent(true));
+    } else {
+      this.props.dispatch(notificationBarPresent(false));


can we grab hasNotificationBar and ONLY call dispatch if nextProps.hasNotificationBar !== this.props.hasNotificationBar? Otherwise we'll be triggering a dispatch on every new prop that comes in

Yes! Very helpful, thanks!

@mattkrick Not sure how to set it the first time without next always equal to the current.

@ackernaut here's the pattern i use:

factor this out into its own method

call that method from componentWillMount and componentWillReceiveProps

from cWRP, pass in this.props and nextProps

from cWM, pass in this.props as the nextProps param and {} for nextProps

Check out MeetingContainer for the pattern, i do this for handleRedirects.

codecov · 2017-03-01T22:39:25Z

Codecov Report

Merging #737 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #737     +/-   ##
=========================================
+ Coverage   56.66%   56.77%   +0.1%     
=========================================
  Files          97       97             
  Lines        1207     1210      +3     
=========================================
+ Hits          684      687      +3     
  Misses        523      523

Impacted Files	Coverage Δ
src/universal/styles/ui.js	`100% <100%> (ø)`	✅

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 a80343b...293fec7. Read the comment docs.

codecov · 2017-03-01T22:39:25Z

Codecov Report

Merging #737 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #737     +/-   ##
=========================================
+ Coverage   56.66%   56.77%   +0.1%     
=========================================
  Files          97       97             
  Lines        1207     1210      +3     
=========================================
+ Hits          684      687      +3     
  Misses        523      523

Impacted Files	Coverage Δ
src/universal/styles/ui.js	`100% <100%> (ø)`	✅

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 bf3385a...abc13ca. Read the comment docs.

mattkrick · 2017-03-01T22:58:45Z

src/universal/components/DashboardWrapper/DashboardWrapper.js

  return (
-    <DashLayoutContainer>
+    <DashLayoutContainer dispatch={dispatch}>


dont pass dispatch to containers, mapStateToProps gives it to em anyways

@ackernaut is this necessary?

mattkrick · 2017-03-01T23:02:27Z

src/universal/modules/notifications/ducks/notificationDuck.js

@@ -0,0 +1,23 @@
+const NOTIFICATION_BAR_PRESENT = 'notifications/NOTIFICATION_BAR_PRESENT';


where does this bar appear? all dashboards? meeting? i think we may wanna refactor it into a dashboard duck. we already have an entity called notifications & having a notifications duck that doesnt pertain to it is gonna get confusing.

ackernaut · 2017-03-01T23:38:56Z

Ok @mattkrick all changes have been made! This PR is ready.

mattkrick · 2017-03-01T23:49:30Z

src/universal/modules/userDashboard/components/UserDashboard/UserDashboard.js

  const title = pathname === '/me' ? 'User Dashboard' : 'User Settings';
  return (
-    <DashboardWrapper location={pathname} title={title}>
+    <DashboardWrapper dispatch={dispatch} location={pathname} title={title}>


mattkrick · 2017-03-01T23:50:16Z

src/universal/redux/makeReducer.js

@@ -35,6 +36,7 @@ const appReducers = {
  [DEFAULT_AUTH_REDUCER_NAME]: auth,
  cashay: cashayReducer,
  form: formReducer.plugin(formPlugin),
+  notifications: notificationReducer,


let's refactor to 'dashboard' or something similar so we don't confuse the notification bar with notifications

ackernaut · 2017-03-02T15:33:57Z

@mattkrick what do you think about DashboardAlert instead of NotificationBar since the content is not really a notification at all, just a cousin sometimes (when pertaining to billing, but not active meetings)...

ackernaut · 2017-03-02T16:37:43Z

Ok @mattkrick third time’s the charm, eh?

Refactored Noti…Bar to DashAlert (including ducks)
Cleaned up unnecessary dispatch props
Improved the DashAlert presence helper method to ensure bool value

mattkrick · 2017-03-02T19:17:50Z

src/universal/modules/newTeam/components/NewTeam/NewTeam.js

@@ -10,7 +10,7 @@ import HTML5Backend from 'react-dnd-html5-backend';
 const NewTeam = (props) => {
  const {dispatch, params: {newOrg}} = props;
  return (
-    <DashboardWrapper title="User Dashboard">
+    <DashboardWrapper dispatch={dispatch} title="User Dashboard">


@ackernaut can we get rid of this?

mattkrick

just 1 itty bitty change!

ackernaut · 2017-03-02T21:23:01Z

👍

jordanh · 2017-03-05T02:36:27Z

Is this ready to merge now?

ackernaut added 2 commits February 23, 2017 13:10

Notification bar state for UI

9c26b5f

Use layout value

0974e05

ackernaut added the pr review label Feb 23, 2017

ackernaut requested a review from mattkrick February 23, 2017 19:20

mattkrick requested changes Feb 24, 2017

View reviewed changes

ackernaut added 5 commits February 24, 2017 14:31

Merge branch 'add-orgs' into add-orgs-modals-take2

3f569ab

Legible UI contstants for modal layout

c0542ab

Merge branch 'master' into add-orgs-modals-take2

b9b8a92

Merge branch 'master' into add-orgs-modals-take2

fd55283

Checking next props to update notibar state

3b80f0d

ackernaut changed the base branch from add-orgs to master March 1, 2017 22:18

ackernaut changed the title ~~Add orgs modals take2~~ Modals and notification bar state Mar 1, 2017

Checking for notification bar

293fec7

ackernaut and others added 2 commits March 1, 2017 16:52

Use state to check for dispatch

8f8185c

rerender less

635a972

mattkrick reviewed Mar 1, 2017

View reviewed changes

ackernaut added 4 commits March 2, 2017 09:35

Merge branch 'master' into add-orgs-modals-take2

0504735

Refactor NotiBar to DashAlert

538ca0c

Tweak dispatch name

404e356

Yank dispatch props, ensure bool

963a542

ackernaut changed the title ~~Modals and notification bar state~~ Modals and DashAlert state Mar 2, 2017

mattkrick reviewed Mar 2, 2017

View reviewed changes

mattkrick requested changes Mar 2, 2017

View reviewed changes

ackernaut added 2 commits March 2, 2017 15:17

Yank dispatch from NewTeam component

e1efcf2

Restored dispatch on NewTeam container

abc13ca

mattkrick approved these changes Mar 5, 2017

View reviewed changes

mattkrick merged commit 0da996b into master Mar 5, 2017

jordanh removed the pr review label Mar 5, 2017

ackernaut deleted the add-orgs-modals-take2 branch March 8, 2017 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modals and DashAlert state #737

Modals and DashAlert state #737

ackernaut commented Feb 23, 2017 •

edited

Loading

mattkrick left a comment

mattkrick Feb 24, 2017

mattkrick Feb 24, 2017

mattkrick Feb 24, 2017

ackernaut Feb 24, 2017

ackernaut Feb 24, 2017

mattkrick Feb 25, 2017

codecov bot commented Mar 1, 2017

codecov bot commented Mar 1, 2017 •

edited

Loading

mattkrick Mar 1, 2017

mattkrick Mar 1, 2017

mattkrick Mar 1, 2017

ackernaut commented Mar 1, 2017

mattkrick Mar 1, 2017

mattkrick Mar 1, 2017

ackernaut commented Mar 2, 2017

ackernaut commented Mar 2, 2017 •

edited

Loading

mattkrick Mar 2, 2017

mattkrick left a comment

ackernaut commented Mar 2, 2017

jordanh commented Mar 5, 2017

		@@ -0,0 +1,23 @@
		const NOTIFICATION_BAR_PRESENT = 'notifications/NOTIFICATION_BAR_PRESENT';

Modals and DashAlert state #737

Modals and DashAlert state #737

Conversation

ackernaut commented Feb 23, 2017 • edited Loading

mattkrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 1, 2017

Codecov Report

codecov bot commented Mar 1, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackernaut commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackernaut commented Mar 2, 2017

ackernaut commented Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

mattkrick left a comment

Choose a reason for hiding this comment

ackernaut commented Mar 2, 2017

jordanh commented Mar 5, 2017

ackernaut commented Feb 23, 2017 •

edited

Loading

codecov bot commented Mar 1, 2017 •

edited

Loading

ackernaut commented Mar 2, 2017 •

edited

Loading