Feature/import new components #52

gavinorland · 2018-08-06T14:00:25Z

These are now operational and have tests, stories and docs. Please let me know if any issues and I'll fix ASAP.

These tests can be further improved, but for the time being they are showing the problem with Table being rendered.

Will further develop tests

Also further develop tests and stories. More work is needed though. Each scenario needs to be covered in tests, usage covered in stories and documentation needs to be done.

Avoids potential HOC styling issue

The conditional seemed to simply return an empty array when the value was already an empty array. Moved check for flag into existing conditional.

skipEmptyRows defaults to false Stories developed Tests passing - but coverage to be increased

I had removed this because it produces a console error. I've been unable to yet track down why Table renders a '0' before its tbody in these conditions - something to look at later. But this is necessary for code coverage.

These are at 100% coverage but more could be tested.

Also docs regen

codecov · 2018-08-06T14:08:09Z

Codecov Report

Merging #52 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #52   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     22    +3     
  Lines         168    202   +34     
  Branches       24     40   +16     
=====================================
+ Hits          168    202   +34

Impacted Files	Coverage Δ
packages/utils/src/tableUtils/index.js	`100% <100%> (ø)`
components/array-object-table/src/index.js	`100% <100%> (ø)`
components/object-table/src/index.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 060244e...51b732b. Read the comment docs.

gavinorland · 2018-08-06T14:11:33Z

I was getting 100% coverage on the components but I see something is bringing coverage down - perhaps the index file - will take a look. - Yes, I have to get some tests in for the utils code - will try to do that ASAP.

stevesims · 2018-08-07T10:09:24Z

components/array-object-table/package.json

+    "@govuk-frederic/utils": "^0.0.1"
+  },
+  "devDependencies": {
+    "@govuk-react/storybook-components": "^0.2.4",


is this really right?
i.e. govuk-react?

I'm not sure if that is right, actually, but that's how it is for all the components in Frederic and that's where that package is. I'm not sure if there should be a duplicate of it in Frederic or if it should be elsewhere.

stevesims

Some suggestions for changes, but they don't have to be done before this is merged in - they can be addressed later

stevesims · 2018-08-07T10:10:13Z

components/array-object-table/package.json

+  "author": "Gavin Orland",
+  "license": "MIT",
+  "homepage": "https://github.com/stevesims/govuk-frederic#readme",
+  "description": "govuk-frederic: Frederic project specific components.",


Should the description for this package file not be about what ArrayObjectTable is and does?

Yes, ideally - this is something that needs doing for all Fred components (and govuk-react ones!). Added #54.

stevesims · 2018-08-07T10:11:06Z

components/array-object-table/src/index.js

+ * <ArrayObjectTable fields={fields} array={array} title={title} skipEmptyRows hideWithNoValues/>;
+ * ```
+ * */
+const ArrayObjectTable = ({ fields = [], array = [], hideWithNoValues = false, skipEmptyRows = false, title, ...props }) => {


might be better/nicer to use defaultProps for hideWithNoValues and skipEmptyRows

stevesims · 2018-08-07T10:11:46Z

components/array-object-table/src/index.js

+ * */
+const ArrayObjectTable = ({ fields = [], array = [], hideWithNoValues = false, skipEmptyRows = false, title, ...props }) => {
+  let rows = rowsFromArray(array, fields, skipEmptyRows);
+  if (!rows.length && !hideWithNoValues) rows = rowsFromArray([{}], fields, false);


euw - single line if statements with no curlies around stuff should be against our linting rules!

stevesims · 2018-08-07T10:14:02Z

components/array-object-table/src/stories.js

+
+stories.addDecorator(WithDocsCustom(ReadMe));
+stories.addDecorator(withKnobs);
+


two lines of whitespace should be against our linting rules, i believe

(i'm not against using two lines like this to help enforce a division, but rules iz rulez)

stevesims · 2018-08-07T10:22:11Z

components/array-object-table/src/test.js

+import Component from '.';
+
+describe('ArrayObjectTable', () => {
+  let fields = [


i'm not keen on this style of declaring variables at the top of the describe, and then using them inside individual tests below.

for instance, fields here ends up getting redefined later - i can guess that's going to be the case as it's defined with a let, but have to trawl down below to see where that happens. redefining fields in a test below also means that other tests after it's redefined need to be aware that it's been redefined.

if there are constant definitions of things to be used through multiple tests then i'd prefer to see them raised outside the describe block and declared as const, preferably given a slightly more descriptive variable name if there's going to be different versions used

similarly there's not much advantage on declaring a single wrapper here - better for that to be const wrapper = inside all the individual tests below. indeed allowing wrapper to persist through multiple tests means that the order of the tests below becomes important - which makes them harder to reason about

additionally if we wish to test different areas of functionality of a component, we can use multiple describe blocks inside the test file

stevesims · 2018-08-07T10:29:50Z

components/object-table/src/index.js

+  object: PropTypes.object,
+  hideWithNoValues: PropTypes.bool,
+  skipEmptyValues: PropTypes.bool,
+  skipMissingKeys: PropTypes.bool,


this prop isn't used

stevesims · 2018-08-07T10:30:59Z

components/object-table/src/test.js

+import Component from '.';
+
+describe('ObjectTable', () => {
+  let fields = [


same comments WRT how the ArrayObjectTable test file is structured apply here too

@stevesims I think I will raise an issue to look at this test structuring in another branch.

stevesims · 2018-08-07T10:32:12Z

packages/utils/package.json

+  "author": "Gavin Orland",
+  "license": "MIT",
+  "homepage": "https://github.com/stevesims/govuk-frederic#readme",
+  "description": "govuk-frederic: Frederic project specific components",


should have better description 😁

stevesims · 2018-08-07T10:37:22Z

packages/utils/src/tableUtils/index.js

+// TODO: ALL THE DOCS
+// TODO: SOME TESTS
+// Empty values and keys are treated the same
+export const rowsFromObject = (object, fields, skipEmptyValues, defaultTransform) => {


i'd have been inclined to have defaultTransform = value => `${value}` here
which would let line 34 below be

(table, { key, heading, names, transform = defaultTransform }) => {

and then allow lines 41-57 to be replaced with just:

const result = transform(object[key], object) || '';

I've made this change, great spot, thanks!

stevesims · 2018-08-07T10:39:20Z

packages/utils/src/test.js

+      { key: 'two', title: 'Two' },
+    ];
+    const result = titlesFromFields(fields);
+    expect(result).toEqual(['One', 'Two']);


on a general coding style point i prefer to see whitespace after variable declarations - one day i'll get around to setting a linting rule to enforce that 😜

#5 becomes ever more pressing! 😛

It’s goooooood! Nice

gavinorland · 2018-08-07T11:05:15Z

@stevesims 👍 Thanks - will work through remaining feedback on this branch post-merge.

gavinorland and others added 30 commits August 6, 2018 12:03

WIP: Add ArrayObjectTable

d8eb971

Remove unnec stories file

08b816e

WIP: Add ObjectTable and its tests

7552076

WIP: Update tests

01b6aef

These tests can be further improved, but for the time being they are showing the problem with Table being rendered.

Remove surplus stories file

c889f4d

Add hideWithNoValues and adjust title implementation

262165e

Update story name

befce0a

ArrayObjectTable refactored

fdededa

Will further develop tests

Updates to ArrayObjectTable

ff486ab

Add skipEmptyRows to ObjectTable too

df67312

Also further develop tests and stories. More work is needed though. Each scenario needs to be covered in tests, usage covered in stories and documentation needs to be done.

Pass remaining props along in ObjectTable

4737b02

Avoids potential HOC styling issue

Remove conditional, adjust check with flag

488fb4d

The conditional seemed to simply return an empty array when the value was already an empty array. Moved check for flag into existing conditional.

Added stories to help with development

6bec7d3

Fix typo

ef8d936

Hide table when there are no values

43c1286

Object table, with all its features, but no docs or tests : ))))

5b63ae2

Update comment about normalisation

16c2811

Important comment for docs

a4e5ec7

Shouting is not a TODO ~

f77fe9e

Yet another doc note

15c9a85

Update docs

edba0f9

ArrayObjectTable updates:

0513249

skipEmptyRows defaults to false Stories developed Tests passing - but coverage to be increased

ArrayObjectTable - Add default render test

8c9ed6f

I had removed this because it produces a console error. I've been unable to yet track down why Table renders a '0' before its tbody in these conditions - something to look at later. But this is necessary for code coverage.

ArrayObjectTable - Bring tests to 100% coverage

eab9831

ArrayObjectTable - Tweak story

4d1285e

ObjectTable - tweak stories

6743e4b

ObjectTable - develop tests

883516c

These are at 100% coverage but more could be tested.

ObjectTable - tweaks to stories

ed31d1c

ObjectTable - Documentation

f5301cc

ArrayObjectTable - Documentation

cbd44a9

Also docs regen

Enable docgen

78dd0b7

gavinorland requested a review from stevesims August 6, 2018 14:00

Loque- and others added 12 commits August 6, 2018 15:32

WIP!! Add support for names

5569713

Nice fix for rows thingy

e10e441

Code style improvement

d692945

TableUtils tests - rowsFromArray

a988733

Add test for rowsFromObject

6d9684f

Fix whitespace issues

f952583

lint fixes for test

0ad6002

TableUtils - function tests

c558fc4

Lurvely tests

a113349

Add consideration for name attr to test

2d97081

TableUtils - Test default values

ed53cb4

TableUtils - Exempt else clause

b3498cc

gavinorland force-pushed the feature/import-new-components branch from e8f8d48 to b3498cc Compare August 6, 2018 17:18

gavinorland and others added 2 commits August 7, 2018 09:50

Remove unnec. util functions import from docs

e0ef0df

Add coverage for null case

9a45366

stevesims reviewed Aug 7, 2018

View reviewed changes

stevesims previously approved these changes Aug 7, 2018

View reviewed changes

Implement feedback on defaultTransform

749101b

It’s goooooood! Nice

Loque- dismissed stevesims’s stale review via 749101b August 7, 2018 10:57

Three minor fixes

51b732b

Loque- approved these changes Aug 7, 2018

View reviewed changes

gavinorland merged commit 350a2d5 into master Aug 7, 2018

This was referenced Aug 7, 2018

Consider changing test structuring #57

Closed

Feature/update new components #60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/import new components #52

Feature/import new components #52

gavinorland commented Aug 6, 2018

codecov bot commented Aug 6, 2018 •

edited

Loading

gavinorland commented Aug 6, 2018 •

edited

Loading

stevesims Aug 7, 2018

gavinorland Aug 7, 2018

stevesims left a comment

stevesims Aug 7, 2018

gavinorland Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

gavinorland Aug 7, 2018

stevesims Aug 7, 2018

stevesims Aug 7, 2018

Loque- Aug 7, 2018

stevesims Aug 7, 2018

gavinorland Aug 7, 2018

gavinorland commented Aug 7, 2018


		stories.addDecorator(WithDocsCustom(ReadMe));
		stories.addDecorator(withKnobs);

Feature/import new components #52

Feature/import new components #52

Conversation

gavinorland commented Aug 6, 2018

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

gavinorland commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesims 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

gavinorland commented Aug 7, 2018

codecov bot commented Aug 6, 2018 •

edited

Loading

gavinorland commented Aug 6, 2018 •

edited

Loading