Select other years in the department preview pages and focus area pages #586

ZeeshaanMaudar · 2019-05-27T15:02:32Z

Still need to work on:

Styling of dropdown
Disabled state
Loading State

schalkventer

Only did a review for FilterDropdown. As requested can you please do the FilterDropdown as a separate PR into master? This PR has SectionHeading files in as well.

schalkventer · 2019-05-27T15:04:49Z

packages/webapp/src/components/FilterDropdown/Markup.jsx

@@ -0,0 +1,49 @@
+import React from 'react';
+import { MenuItem, CssBaseline } from '@material-ui/core';


Why are you importing CssBaseline?

I tried something and forgot to remove it at a later stage. Removed.

schalkventer · 2019-05-27T15:05:39Z

packages/webapp/src/components/FilterDropdown/Markup.jsx

+  if (loading) {
+    return (
+      <MenuItem value="boom" displayEmpty name="something">
+        {callSpinner()}


You can just inline the output here since you're only using it once.

Prettier does not allow for it.

schalkventer · 2019-05-27T15:09:04Z

packages/webapp/src/components/FilterDropdown/Markup.jsx

+  }
+
+  return options.map(({ id: idVal, name }) => {
+    const selectedKey = options.findIndex(({ id }) => id === idVal);


What does selectedKey do? Why not use the id as the key?

schalkventer · 2019-05-27T15:11:16Z

packages/webapp/src/components/FilterDropdown/Markup.jsx

+
+const callOptions = (options, selected, eventHandler, greyTheme, disabled, loading) => (
+  <SelectPreview
+    {...{ greyTheme }}


What does greyTheme do? Perhaps just add a prop to a component called primary which is a boolean. If primary is true then apply the white styling.

Swapped to primary

schalkventer · 2019-05-27T15:11:35Z

packages/webapp/src/components/FilterDropdown/Markup.jsx

+const callOptions = (options, selected, eventHandler, greyTheme, disabled, loading) => (
+  <SelectPreview
+    {...{ greyTheme }}
+    disabled={disabled}


Destructure this.

schalkventer · 2019-05-27T15:18:45Z

packages/webapp/src/components/FilterDropdown/index.jsx

+  }
+
+  eventHandler(e) {
+    const { updateUrl, year } = this.props;


Why do you have updateUrl and year in here? FilterDropdown should not know anything about the context outside of it.

schalkventer · 2019-05-27T15:19:24Z

packages/webapp/src/components/FilterDropdown/index.jsx

+
+  eventHandler(e) {
+    const { updateUrl, year } = this.props;
+    const { selected } = this.state;


Remove all the history state logic below.

schalkventer · 2019-05-27T15:19:44Z

packages/webapp/src/components/FilterDropdown/index.jsx

+      ...props,
+      eventHandler: events.eventHandler,
+      selected: state.selected,
+      year: props.year,


Same issue as above.

schalkventer · 2019-05-27T15:21:38Z

packages/webapp/src/components/FilterDropdown/schema.ts

+
+// Type: Tselected
+/**
+ * This is the selected text for the menu item inside of the drop down. Being unique, it


schalkventer · 2019-05-27T15:22:31Z

packages/webapp/src/components/FilterDropdown/styled.js

+
+import styled from 'styled-components';
+
+const FormContainer = styled.div`


This needs to be removed.

…nto filter-dropdown

jbothma · 2019-06-05T05:47:04Z

packages/webapp/src/components/BarChart/Presentation.jsx

+const Markup = ({ items }) => (
+  <BarChartContainer>
+    {items.map((props, index) => (
+      <Bar key={items[index].amount} {...{ index, items, ...props }} />


this is scary - we're assuming amounts will be unique? what happens if they're not?

jbothma · 2019-06-05T06:08:48Z

packages/webapp/src/components/ChartSection/Markup.jsx

@@ -41,8 +67,14 @@ const callButtonExplore = (url, color, verb, subject) => {
  );
 };

-const callDetails = (selected, verb, subject) => {
-  const { name, value, url, color } = selected;
+const callAmount = (value, loading) => (


what do we mean when we call these functions call...? It doesn't make sense to me as an old fart. You call a function - it's weird to call callFoo. This function does call something (like most functions) - it calls the constructor of Amount - would it make more sense to call it createAmount? constructAmount might sound a bit like it's a factory for Amounts.

In fact it creates Amount and decides what content should be inside it.

Is calling it call... a common pattern? I'm not saying it has to be changed, it's just sounds like we're writing a function to call a function and we call it callfunction.

jbothma · 2019-06-05T06:13:33Z

packages/webapp/src/components/ChartSection/Notices.jsx

-    {callText(notices)}
-  </Wrapper>
-);
+const Notices = ({ notices }) => <Wrapper>{callText(notices)}</Wrapper>;


This seems like overzealous one-liners because we can.

Surely it'd be easier to understand what markup we're building here with

const Notices = ({notices}) => <Wrapper> notices.map(notice => <Text key={notice}>{notice}</Text>) </Wrapper>;

like...the way it's written, if you want to know what Notices does, you're like it's a wrapper, and now I need to go read that function over there

what's Notices actually? It's a list of notices...

jbothma · 2019-06-05T08:38:59Z

packages/webapp/src/components/ContentFilterHeading/calcPrettyName.js

-const calcPrettyName = government => {
-  if(government === 'south-africa') {
+const calcPrettyName = title => {
+  if (title === 'south-africa') {


This doesn't make sense any more and is probably something the view should pass in rather than have such domain-specific knowledge in the component

jbothma · 2019-06-05T15:40:55Z

packages/webapp/src/views/DepartmentSummary/calcFineprint.js

@@ -0,0 +1,13 @@
+const calcFineprint = year => {
+  if (year === '2018-19') {


This needs to be data-driven

jbothma · 2019-06-05T15:42:12Z

packages/webapp/src/views/DepartmentSummary/index.jsx

+      }))
+    : [];
+
+  console.log(departmentArray, '3333333333333333');


jbothma · 2019-06-05T15:42:32Z

packages/webapp/src/views/DepartmentSummary/index.jsx

+      selectionDropdown,
+      yearDropdown,
+    },
+    // national,


what's this for?

jbothma · 2019-06-05T15:46:32Z

packages/webapp/src/views/DepartmentSummary/useUpdateYearsData.js

+import createFetchDataFn from './createFetchDataFn';
+import calcValidYearsRange from './calcValidYearsRange';
+
+const createEndpointUrl = year => `/json/${year}/focus.json`;


focus.json in DepartmentSummary?

jbothma · 2019-06-05T16:17:34Z

packages/webapp/src/views/FocusAreaSummary/schema.ts

@@ -0,0 +1,164 @@
+import faker from 'faker';


mock data generation in production namespaces is a very big warning sign

While the schemas are only used in test code, let's move all schemas to the test namespaces.

When we want to use types in production code, we can pull the types out to the prod namespace and import them form the test code

jbothma · 2019-06-14T14:28:55Z

packages/webapp/src/views/DepartmentSummary/types.ts

+ */
+export type TgetDepartmentsData = (TgetDepartmentsDataProps) => void;
+
+// Type: TconstructGetFocusAreaDataProps


Guys, I don't think these types.ts files is the way to go.

There's a reason why we tend to put types by the functions and methods where they're used. Code locality is a Big Thing.

I'm jumping around about 5 files trying to figure out where the missing piece is and the types are in their own file?
Is this the goal, or is this just a stopgap to start adding types but eventually they'll be located adjacent to the relevant function?

ZeeshaanMaudar added 11 commits May 24, 2019 14:46

start creating new component and schema

f90175f

delete story

474ec8c

change to focus preview pages

08963ab

start creating FilterDropdown

043ffbe

start creating FilterDropdown

779024e

start adding state

6c8adc8

styling for theme

20c8e85

refactor

f04eef0

start adding loading state

72cf954

Start adding Spinner

4458cf6

remove redundant stories

8d9c7a6

schalkventer suggested changes May 27, 2019

View reviewed changes

ZeeshaanMaudar added 18 commits May 28, 2019 10:00

remove CssBaseline

247e164

remove selectedKey and used id instead

2379e56

rename eventHandler to onSelectionChange

c3a1032

remove check for options available.

29367a6

remove redundant updateUrl and year props

24416da

remove year props

c7a1e40

change greyTheme bool to primary

b3aa9a5

fix styling

c3a2e88

add disabled styling

c1b1238

add loading state

38c484e

add logic for disabled

e956236

add new stories

500330c

remove state

bebfa63

Prettier fixes

6c74e2b

update schema

d25f385

Merge branch 'master' of github.com:vulekamali/static-budget-portal i…

449785b

…nto filter-dropdown

create stories for FilterDropdown

df8d6cb

add extra story for dropdown menu item disabled

6b55c65

ZeeshaanMaudar added 3 commits June 4, 2019 14:22

remove border

0ab51cd

change background color

b596284

prettifying

5fe7d9b

jbothma reviewed Jun 5, 2019

View reviewed changes

Fix import to make things build again

7aecfd4

jbothma suggested changes Jun 5, 2019

View reviewed changes

schalkventer added 14 commits June 11, 2019 14:45

Merges master

f0cd6b5

Updates enivornment configuration

8b8d47d

Add global project types

46c8fec

Fixes React warning

e0099a5

Fixes additional React warning

df3ec02

Removes key from BarChart

702548d

Moves pretty department name function up to view level

c516f7b

Updates DepartmentSummary view

5c74d11

Updates FocusAreaSummary view

0a692bc

Merges upstream

a8ef3f6

Minor changes to views

8871bd0

Create both summary page transformers

a005487

Add routing logica and Jekyll markup

3316177

Remove console.log

67dc4fc

jbothma changed the title ~~Filter dropdown~~ Select previous years' data in the department preview pages and focus are pages Jun 14, 2019

jbothma changed the title ~~Select previous years' data in the department preview pages and focus are pages~~ Select previous years in the department preview pages and focus are pages Jun 14, 2019

jbothma changed the title ~~Select previous years in the department preview pages and focus are pages~~ Select other years in the department preview pages and focus area pages Jun 14, 2019

Fix missing explore button

9cfb4c7

jbothma reviewed Jun 14, 2019

View reviewed changes

jbothma added 2 commits June 14, 2019 16:35

Fix department preview story probably copy pasta

1874849

Fix json endpoint - copy pasta

d4c95e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select other years in the department preview pages and focus area pages #586

Select other years in the department preview pages and focus area pages #586

ZeeshaanMaudar commented May 27, 2019

schalkventer left a comment

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

schalkventer May 27, 2019

schalkventer May 27, 2019

ZeeshaanMaudar May 28, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 5, 2019

jbothma Jun 14, 2019

		@@ -0,0 +1,49 @@
		import React from 'react';
		import { MenuItem, CssBaseline } from '@material-ui/core';


		import styled from 'styled-components';

		const FormContainer = styled.div`

		@@ -0,0 +1,13 @@
		const calcFineprint = year => {
		if (year === '2018-19') {

Select other years in the department preview pages and focus area pages #586

Are you sure you want to change the base?

Select other years in the department preview pages and focus area pages #586

Conversation

ZeeshaanMaudar commented May 27, 2019

schalkventer 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

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

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

Choose a reason for hiding this comment