2.0 port of the sunburst + some cleanup. #33

MaybeJustJames · 2019-10-18T09:31:23Z

Closes #30

Attempting to mimic current style

Transpiled output should not be in git

- Also change parameter name for sunburstNode.createNodes from node to nodes

* Now jquery plugin is not going to be ported

Also remove yarn lock file

Also refactor color manipulation code from util into color

And minor quality of life improvements

pverscha

Hey @MaybeJustJames ,

Thank you for this PR :) I do have some suggestions / comments that you should take a look at, if possible.

Some general remarks:

Try not to prefix interfaces with "I". This is a coding-style that's not commonly used in TypeScript / JavaScript environments and that's even discouraged by Angular / React. (see this for more information: Prohibition against prefixing interfaces with "I" microsoft/TypeScript-Handbook#121 (comment))
There's still a tslint.json file present in the repository, although we are using ESLint. Note that TSLint used to be the default linter for TypeScript, but that it is currently deprecated and should be completely replaced by ESLint.
Run an automatic ESLint fix afterwards to clean up the indentation.
I'm not a big fan of declaring functions as const's throughout the code (it can occasionally be handy, but I think it should be avoided in general). Instead of something like this:

/**
 * Checks that `child` is within the hierarchial depth limit and is within
 * the same (sub-)tree as `ancestor`.
 * @param maxGeneration The (absolute) hierarchial depth limit.
 * @return true iff the above conditions are satisfied.
 */
const displayableFromAncestor: (ancestor: d3.HierarchyNode<Node>,
                                child: d3.HierarchyNode<Node>,
                                maxGeneration: number) => boolean
  = (ancestor: d3.HierarchyNode<Node>,
     child: d3.HierarchyNode<Node>,
     maxGeneration: number): boolean =>
  (child.depth < maxGeneration) &&
  (Data.ancestorOf(ancestor, child)
   .orElse(Infinity) < maxGeneration);

I suggest to reformat this to something clearer:

function displayableFromAncestor(
    ancestor: d3.HierarchyNode<Node>,
    child: d3.HierarchyNode<Node>,
    maxGeneration: number
): boolean {
    return child.depth < maxGeneration && (Data.ancestorOf(ancestor, child).orElse(Infinity) < maxGeneration);
}

This way, we don't have to define the parameters of this function twice (once in the type signature and once in the actual function) and the start and end of the function can easily be spotted.

How should we proceed from here? Do you still have time to look into these things or should we try to update the PR according to my comments and merge it ourselves?

I would like to thank you again for your help in updating our package to TypeScript / D3 v5 :)

pverscha · 2020-11-23T08:01:17Z

src/lib/sunburst/node.ts

+ super(node);
+ }
+
+ public static new(node: BasicNode = emptyBasicNode()): SunburstNode {


What's the added value of this function? Can't we just use the constructor to create new SunburstNode's since all this function does is call the constructor?

pverscha · 2020-11-23T09:02:42Z

src/lib/breadcrumb.ts

+ const clr: (node: d3.HierarchyNode<Node>) => string
+ = (node: d3.HierarchyNode<Node>): string => {
+ const c = color !== undefined ? color(node) : "black";
+ console.log(`background color: ${c}`);


This log should be removed.

pverscha · 2020-11-23T09:15:46Z

src/lib/breadcrumb.ts

+/**
+ * Given some settings, bake a function that generates HTML for a breadcrumb.
+ */
+const createCrumb: (genClassName: (name: string) => string,


I suggest to change this and declare this as a normal function, like so:

function createCrumb( genClassName: (name: string) => string, text?: (node: d3.HierarchyNode<Node>) => string, color?: (node: d3.HierarchyNode<Node> => string ) { ... }

It's quite hard to read this function right now due to all of the nested arrow-functions and inconsistent indentation.

pverscha · 2020-11-23T09:21:02Z

src/lib/breadcrumb.ts

+ }
+
+ /**
+ * Update the rendered breadcrumbs with `data` as as the node deepest


Something is wrong with this comment, can you take another look and update it? :)

pverscha · 2020-11-23T09:23:29Z

src/lib/breadcrumb.ts

+ this.colour !== undefined
+ ? this.colour(d)
+ : Breadcrumb.DEFAULT_COLOUR)


Also place this on one line, similar to line 110.

pverscha · 2020-11-23T10:10:21Z

src/lib/colorPalette.ts

@@ -0,0 +1,31 @@
+import { scaleOrdinal, ScaleOrdinal } from "d3";


Is there an advantage in using these colour palette's over the built-in D3 palettes?

pverscha · 2020-11-23T10:12:02Z

src/lib/color.ts

+ * Formula: sum(squared components)/# of components
+ * TODO: Make a weighted average version.
+ */
+const averageColor: (colours: OptionalColor[]) => OptionalColor


This function is not being used and can maybe be removed?

pverscha · 2020-11-23T10:13:36Z

src/lib/data.ts

+ = (datum: Node, counter: (d: Node) => number): number => {
+ if (datum.children !== undefined) {
+ return datum.children.map((v: Node): number => count(v, counter))
+ .reduce(((a: number, b: number): number => a + b), 0);


Use D3's built-in sum function here.

pverscha · 2020-11-23T10:17:30Z

src/lib/settings.ts

+
+ public readonly abstract className: string; // The visualization class: for styling
+
+ public constructor(height?: number, width?: number, enableTooltips?: boolean) {


I think it might be better to change this constructor to:

public constructor(height: number = Settings.DEFAULT_SIZE, width: number = Settings.DEFAULT_SIZE, enableTooltips: boolean = true)

pverscha · 2020-11-23T11:49:06Z

src/lib/styles.ts

+
+/**
+ * IStyleConstraints data constructor
+ * Use with care!


Can you elaborate here and explain why this should be used with care?

pverscha · 2021-01-26T12:17:48Z

Hi @MaybeJustJames,

Thank you for your PR :) We've incorporated the work you made in this PR, together with some new changes (since the D3 API changed again) in a new PR #36 that has been merged with the codebase by now.

MaybeJustJames added 30 commits August 29, 2019 10:12

Set up automatic code formatting

add6959

Attempting to mimic current style

Extract color palettes from src-old/sunburst/sunburst.js

252c91e

Script re-implementation of src-old/sunburst/sunburstNode.js

4ef38f0

Correct formatting

d3fc996

Initial generic settings implementation

a76e442

Remove dist/ from the git index

b7463cf

Transpiled output should not be in git

Ignore dist/ files and backup files

ad2f334

Fix type error (by casting) in maxCountHeap

d3ef200

Complete initial implementation of SunburstSettings and begin tests

909cf4a

Initial template implementation of sunburst class

e9500d9

Ensure tests for SunburstSettings pass

596def9

Mark rerootCallback as optional

dc04106

Implement custom Optional<> type

6e0234b

- Also change parameter name for sunburstNode.createNodes from node to nodes

Add tslint configuration

604e1b1

Reduce SunburstSettings implementation complexity

0fa328a

Correct import

eabe327

Correct some tslint warnings

7934d99

Test SunburstNode

5c1c3d5

Extra type checking

40635c2

Trying to make sunburst compile

84e898f

Fix some type warnings

a0bedce

Sunburst now compiles

4faf904

Example displays an SVG

eb939ae

* Now jquery plugin is not going to be ported

Clean up package.json

00e86c6

Fix build

2bbe416

Also remove yarn lock file

Typescript version now displays a sunburst image

43ec2e7

Implement some useful mathematical functions

d9414d3

Fix a logic bug in Optional::isPresent

5042cbf

Create a Color manipulation library

a08cfbd

Also refactor color manipulation code from util into color

Fix Optional class of() factory method

c182f6a

MaybeJustJames added 23 commits September 26, 2019 13:04

Got animation somewhat working

7863653

Trying to work out rescaling

42b8d2b

Initial animation working

54401d3

Small fix for parentNode in animation

b5f96fb

Add some tests for Data

84da015

Animate text labels correctly

b11a58c

Fix for color tests

5cdbc59

Scale the radius correctly even when animating

3ed72cb

Now have label display based on node size correct

4902622

Hide labels that do not fit

f78dfdc

Compute reasonable text label sizes

9944b2d

Extract breadcrumb logic

adf3e72

Extract generic tooltip logic

c47e84f

Extract styling functions

8acf983

Extracted all label styliung functions

3d07d03

Extract the only node styling function

c15f549

A bit of code organising and documentation for styles

42b2b15

Add some documentation

94168af

Minor reorganisation

33f1074

Keep tslint as happy as can be

97c7763

Code cleanup

d12b401

Small reorganisation and docs

b793f42

Basic example showing multiple sunburst visualisations

f3bb68c

And minor quality of life improvements

MaybeJustJames requested a review from bmesuere October 18, 2019 09:31

MaybeJustJames assigned bmesuere Oct 18, 2019

MaybeJustJames requested a review from pverscha June 30, 2020 10:52

pverscha requested changes Nov 23, 2020

View reviewed changes

pverscha closed this Jan 26, 2021

bmesuere deleted the 2.0_sunburst branch March 9, 2021 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.0 port of the sunburst + some cleanup. #33

2.0 port of the sunburst + some cleanup. #33

MaybeJustJames commented Oct 18, 2019

pverscha left a comment

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha Nov 23, 2020

pverscha commented Jan 26, 2021

		@@ -0,0 +1,31 @@
		import { scaleOrdinal, ScaleOrdinal } from "d3";


		public readonly abstract className: string; // The visualization class: for styling

		public constructor(height?: number, width?: number, enableTooltips?: boolean) {

2.0 port of the sunburst + some cleanup. #33

2.0 port of the sunburst + some cleanup. #33

Conversation

MaybeJustJames commented Oct 18, 2019

pverscha 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

pverscha commented Jan 26, 2021