Fill out GroupDetail page #211

PtrTeixeira · 2017-02-07T22:02:18Z

This is work for the GroupDetail page of Baragon's UI, in the process of porting it from Backbone to React + Redux.

This puts the _visual_ part of the Group Detail page together. It doesn't have any interactivity yet and isn't actually connected to the data stores of the application as a whole, but it has the html together.

The slow part here was that there was an action being called but it was missing a reducer, so the results of the action weren't reflected in the state of the application. This bumps that so the `FetchGroup` action does get mirrored in application state, and everything turns out fine.

Wire together the modal to remove a traffic source.

ssalinas

Overall pretty good, major comment was just on the organization + splitting into different components. I wouldn't worry too much about overall style for now, we can always make things look nicer later. For now can just make sure they function

ssalinas · 2017-02-09T13:41:35Z

BaragonUI/app/components/common/modalButtons/AddTrafficSourceModal.jsx

+
+const mapDispatchToProps = (dispatch, ownProps) => ({
+  addTrafficSource: (data) => dispatch(AddTrafficSource.trigger(ownProps.groupName, data))
+      .then(response => dispatch(FetchGroup.trigger(ownProps.groupName)))


rather than specifically specifying this here, we can have it as the then prop when using this component in GroupDetail.jsx. That way the parent component calling it can control what happens (i.e. if called from the group detail page, we definitely want to reload, if called from somewhere else, maybe we want a different action).

ssalinas · 2017-02-09T13:42:32Z

BaragonUI/app/components/common/modalButtons/ModifyCountModal.jsx

+
+const mapDispatchToProps = (dispatch, ownProps) => ({
+  modifyCount: (data) => dispatch(ModifyTargetCount.trigger(ownProps.groupName, data.newCount))
+      .then(response => dispatch(FetchGroupTargetCount.trigger(ownProps.groupName)))


similar to what i said in the traffic source modal here

ssalinas · 2017-02-09T13:43:40Z

BaragonUI/app/components/common/modalButtons/RemoveBasePathButton.jsx

+
+import { getClickComponent } from '../modal/ModalWrapper';
+
+import AddTrafficSourceModal from './AddTrafficSourceModal';


wrong import

ssalinas · 2017-02-09T13:44:08Z

BaragonUI/app/components/common/modalButtons/RemoveBasePathModal.jsx

+
+const mapDispatchToProps = (dispatch, ownProps) => ({
+  removeBasePath: () => dispatch(RemoveBasePath.trigger(ownProps.groupName, ownProps.basePath))
+      .then(response => dispatch(FetchGroupBasePaths.trigger(ownProps.groupName)))


same here, pass it in the then prop instead

ssalinas · 2017-02-09T13:44:43Z

BaragonUI/app/components/common/modalButtons/RemoveKnownAgentButton.jsx

+import { getClickComponent } from '../modal/ModalWrapper';
+
+import RemoveKnownAgentModal from './RemoveKnownAgentModal';
+import AddTrafficSourceModal from './AddTrafficSourceModal';


import not needed

ssalinas · 2017-02-09T13:46:06Z

BaragonUI/app/components/common/modalButtons/RemoveKnownAgentModal.jsx

+const mapDispatchToProps = (dispatch, ownProps) => ({
+  removeKnownAgent: (data) => dispatch(RemoveKnownAgent.trigger(ownProps.groupName, ownProps.agentId))
+      .then(response => dispatch(FetchGroupKnownAgents.trigger(ownProps.groupName)))
+      .then(response => dispatch(FetchGroupAgents.trigger(ownProps.groupName)))


these go in the then prop, but you also wouldn't need to refetch the active agents here, they are separate from the known agents list

ssalinas · 2017-02-09T13:49:57Z

BaragonUI/app/components/groupDetail/GroupDetail.jsx

+      .value();
+  }
+
+  asGroups(arr, itemRenderer) {


this was related to a styling thing from the old ui, if we can find out an easier way to display the large lists of items so they fit easily on the screen let's go for it :) (i.e. something where it automatically overflows to new lines rather than chunking it into columns)

ssalinas · 2017-02-09T14:02:15Z

BaragonUI/app/components/groupDetail/GroupDetail.jsx

  };
+
+  render() {
+    return (


the general html generated by this is fine, but a comment on the organization here. Right now this is a lot of code in one component. I would suggest breaking things up into smaller components (that can also live in this same folder). For example, you could have a <TrafficSources> component that takes a prop of the traffic source data. You could do similar for an Agents or a KnownAgents component or a BasePaths component.

Not only does this help us to read and organize things a bit better, it also means we can reuse things like that elsewhere in the ui if we need to.

for example a snippet of the code would read more like:

<div className="row"> <TrafficSources trafficSources={this.props.trafficSources} group={this.props.group} /> </div>

ssalinas · 2017-02-09T14:03:24Z

BaragonUI/app/components/groupDetail/GroupDetail.jsx

+  trafficSources: Utils.maybe(state, ['api', 'group', ownProps.params.groupId, 'data', 'trafficSources']),
+  editable: true // TODO,


we can actually grab this from window.config which is set up in our index.html. That is globally available to us to check.

I still need to implement the edit/no-edit piece of things, but a simple if (window.config.allowEdit) check should suffice when needed

Split the single large GroupDetail component into a single component connected to the state object and a lot of smaller pure components. In doing so, also injected the refresh behavior from the top-level `GroupDetail` component into the modals, rather than having the modals themselves perform the data fetches.

This mostly involved adding property types to the components that I had made and not added property types to. In some cases it did involve correcting things that were imported and not used, or were the incorrect imports for a file.

ssalinas

LGTM once those last comments are addressed 👍

ssalinas · 2017-02-16T21:04:38Z

BaragonUI/app/components/common/modalButtons/RemoveKnownAgentModal.jsx

+const mapDispatchToProps = (dispatch, ownProps) => ({
+  removeKnownAgent: (data) => dispatch(RemoveKnownAgent.trigger(ownProps.groupName, ownProps.agentId))
+      // .then(response => dispatch(FetchGroupKnownAgents.trigger(ownProps.groupName)))
+      // .then(response => dispatch(FetchGroupAgents.trigger(ownProps.groupName)))


can we remove the comments here

ssalinas · 2017-02-16T21:09:48Z

BaragonUI/app/components/groupDetail/GroupDetail.jsx

+const mapDispatchToProps = (dispatch, ownProps) => ({
+  afterModifyTargetCount: () => dispatch(FetchGroupTargetCount.trigger(ownProps.params.groupId)),
+  afterAddTrafficSource: () => dispatch(FetchGroup.trigger(ownProps.params.groupId)),


it'd probably be clearer to name these by what function they are performing, not by the context they are being used in.

i.e. fetchGroup which can then be used in both the then for adding and removing traffic source, rather than defining those separately.

You can also do something a bit more efficient for ones with multiple calls (like the afterRemoveKnownAgent currently is). Dispatch returns a promise. So, rather that triggering one, waiting for response, and triggering another, we could do something like

fetchAllAgents: () => Promise.all([ dispatch(FetchGroupKnownAgents.trigger(ownProps.params.groupId), dispatch(FetchGroupAgents.trigger(ownProps.params.groupId) ])

The Promise.all returns a single promise that is completed when all of the contained promises return

ssalinas · 2017-02-16T21:12:03Z

BaragonUI/app/components/groupDetail/util.jsx

+// array is as long as possible
+// [1, 2, 3, 4, 5, 6, 7], 2 -> [[1, 2], [3, 4], [5, 6], [7]]
+// Precondition: size != 0
+function chunk(arr, size) {


similar to the comment on the other PR, let's just keep a single utils file that can be shared everywhere

Involved some more style changes now that I have the linter running in Atom. Also, more specifically, involved removing some commented out code that I had accidentally just left laying around, rename the functions in `GroupDetail` from `after...` to `refresh...` to reflect what they did rather than how they were going to be used, and move the utility code that I had been re-using into a common file. W/r/t the second point, I renamed the functions in the `GroupDetail` page but not in any of the subcomponents - eg, `BasePaths`, &c. My justification here is that those functions are getting injected into the subcomponents. They shouldn't know anything about what they do - the function's internals - but should only care about how they are going to use them. To me it makes sense to keep them named `after...` in the subcomponents.

ssalinas · 2017-02-21T14:00:46Z

👍 Let's fix the merge conflicts and get this one merged

PtrTeixeira added 6 commits February 7, 2017 15:16

First pass at design portion

e472823

This puts the _visual_ part of the Group Detail page together. It doesn't have any interactivity yet and isn't actually connected to the data stores of the application as a whole, but it has the html together.

Add RemoveTrafficSource modal

530a2df

Wire together the modal to remove a traffic source.

Add RemoveKnownAgent modal

90de8c1

Add RemoveBasePath modal

b0ddac2

Add ModifyTargetCount modal

9f3a1d7

ssalinas requested changes Feb 9, 2017

View reviewed changes

PtrTeixeira changed the title ~~[WIP] Fill out GroupDetail page~~ Fill out GroupDetail page Feb 10, 2017

PtrTeixeira added 2 commits February 10, 2017 14:38

Remove unused import

da5a90a

Small style fixes

743525e

This mostly involved adding property types to the components that I had made and not added property types to. In some cases it did involve correcting things that were imported and not used, or were the incorrect imports for a file.

ssalinas requested changes Feb 16, 2017

View reviewed changes

PtrTeixeira merged commit 629fcaa into react Feb 21, 2017

PtrTeixeira deleted the group-detail branch February 21, 2017 15:39

ssalinas modified the milestone: 0.5.0 May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill out GroupDetail page #211

Fill out GroupDetail page #211

PtrTeixeira commented Feb 7, 2017

ssalinas left a comment

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas Feb 9, 2017

ssalinas left a comment

ssalinas Feb 16, 2017

ssalinas Feb 16, 2017

ssalinas Feb 16, 2017

ssalinas commented Feb 21, 2017


		import { getClickComponent } from '../modal/ModalWrapper';

		import AddTrafficSourceModal from './AddTrafficSourceModal';

		trafficSources: Utils.maybe(state, ['api', 'group', ownProps.params.groupId, 'data', 'trafficSources']),
		editable: true // TODO,

Fill out GroupDetail page #211

Fill out GroupDetail page #211

Conversation

PtrTeixeira commented Feb 7, 2017

ssalinas 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssalinas 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

ssalinas commented Feb 21, 2017