fix: add enhancements #17, #12 (custom settings) #24

ahoak · 2020-11-24T22:39:54Z

Add settings property for allowing custom display of tables and complete following tickets:

Unify Background Color for Neighbor Column Bars Unify Background Color for Neighbor Column Bars #17
Configurable columns in hierarchy browser tables Configurable columns in hierarchy browser tables #12
Use command pattern for table header bar buttons Use command pattern for table header bar buttons #13 (partial)

darthtrevino · 2020-11-24T23:08:28Z

packages/hierarchy-browser/package.json

@@ -22,14 +22,17 @@
 		"not op_mini all"
 	],
 	"dependencies": {
+		"@essex-js-toolkit/hooks": "workspace:packages/hooks",
 		"@thematic/core": "^0.8.0",
 		"@thematic/fluent": "^0.8.0",
 		"@thematic/react": "^0.8.0",
 		"@types/d3-array": "^2.2.0",


@types/<x> dependencies should go into devDeps and peerDeps

packages/hierarchy-browser/package.json

packages/hierarchy-browser/src/CommunityCard/CommunityCard.tsx

packages/hierarchy-browser/src/hooks/useUpdatedCommunityProvider.ts

darthtrevino · 2020-11-24T23:14:36Z

packages/hierarchy-browser/src/hooks/useUpdatedHierarchyProvider.ts

+			const isLoaded = hierachyDataProvider.updateNeighbors(neighbors)
+			setIsNeighborsLoaded(isLoaded) // trigger neighbor refresh
+		}
+	}, [neighbors])


hierarchyDataProvider dep?

darthtrevino · 2020-11-24T23:15:21Z

packages/hierarchy-browser/src/hooks/useUpdatedHierarchyProvider.ts

+] {
+	const [isNeighborsLoaded, setIsNeighborsLoaded] = useState<boolean>(false)
+	useEffect(() => {
+		hierachyDataProvider && hierachyDataProvider.updateCommunities(communities)


The ?. operator can be used here instead of steeltoeing:
hierarchyDataProvider?.updateCommunities(...)

darthtrevino · 2020-11-24T23:15:32Z

packages/hierarchy-browser/src/hooks/useUpdatedHierarchyProvider.ts

+		hierachyDataProvider && hierachyDataProvider.updateCommunities(communities)
+	}, [communities])
+	useMemo(() => {
+		hierachyDataProvider && hierachyDataProvider.updateEntities(entities)


Here as well

darthtrevino · 2020-11-24T23:15:46Z

packages/hierarchy-browser/src/CommunityCard/CommunityCard.tsx

@@ -98,15 +110,19 @@ export const CommunityCard: React.FC<ICommunityCardProps> = memo(
 					filterProps={filterProps}
 					getEntityCallback={loadMore}
 					level={level}
+					fontStyles={fontStyles}
+					controls={controls}
 				/>
 				<Flex>
 					<Content style={contentStyle}>
 						{entities && entities.length > 0 ? (


entities?.length

darthtrevino · 2020-11-25T00:45:18Z

packages/hierarchy-browser/src/CommunityCard/CommunityCard.tsx

 	}: ICommunityCardProps) {
-		const [dataProvider] = useState<CommunityDataProvider | undefined>(
+		const [dataProvider] = useState<CommunityDataProvider>(


useMemo<CommunityDataProvider>(() => new CommunityDataProvider(...), [/* no deps intentionally */])

so if you use no deps on a useMemo, it will only run once? What is the advantage of using useMemo over useState?

It's a readability thing. useState implies that you have something that's going to be mutated by your logic. Because there's no mutation involved a useMemo communicates your intent better.

darthtrevino

Taking another pass at looking through the code

darthtrevino · 2020-11-25T04:05:48Z

packages/hierarchy-browser/package.json

@@ -39,14 +37,19 @@
 		"@babel/plugin-transform-runtime": "^7.11.5",
 		"@essex/scripts": "^11.0.1",
 		"@fluentui/react": "^7.146.1",
+		"@types/d3-array": "^2.2.0",


All of these @types/ libraries should be in peerDeps as well

darthtrevino · 2020-11-25T04:08:20Z

packages/hierarchy-browser/src/hooks/useControls.ts

+					? controls.showMembership
+					: membership
+			filterData =
+				controls.showFilter !== undefined ? controls.showFilter : filterData


The safest check here is x != null, which accounts for both null and undefined

darthtrevino · 2020-11-25T04:09:15Z

packages/hierarchy-browser/src/hooks/useLoadEntitiesOnMountEffect.ts

-	useLayoutEffect(() => {
-		if (isOpen && !entitiesLoaded) {
+	useEffect(() => {
+		if (isOpen && !entitiesLoaded && size && size > 0) {


I don't think you need the && size check. Undefined and null both return false for > 0

darthtrevino · 2020-11-25T04:09:44Z

packages/hierarchy-browser/src/hooks/useSettings.tsx

+			settings && settings.isOpen !== undefined ? settings.isOpen : index === 0,
+		[settings],
+	)
+	const visibleColumns = useMemo(() => settings && settings.visibleColumns, [


settings?.visibleColumns

darthtrevino · 2020-11-25T18:18:32Z

packages/hierarchy-browser/src/HierarchyBrowser.tsx

 		() => new HierarchyDataProvider(communities, entities, neighbors),
+		[],


can you add a comment inside the deps array that we're ignoring communities, entities and neighbors intentionally?

okay do you want me to add a tslint ignore line as well?

if it's popping a linter error, then yes. But even if not, I think a comment inside the dep array that it was left intentionally empty will help future readers understand the decision

It is only a warning in the linter so it doesn't effect the build but I can add it.

The linter warnings are worth working through and resolving. Some of them may not be worth enabling by default. The issues it's reporting on about hooks dependencies are definitely worth taking a close look at

I just disabled the rule for the block of code here for both dataproviders. I think that there maybe other packages that have a similar issue for your awareness in case we decide to do clean up on those later linting errors later.

darthtrevino · 2020-11-30T17:06:31Z

packages/hierarchy-browser/src/common/dataProviders/CommunityDataProvider.ts

+			const nextNeghbors = this.loadNeighborsAsync(params)
+			return nextNeghbors || []
+		} catch (err) {
+			throw Error(


this swallows the source error, which may be useful for debugging - maybe we should log it out before throwing a new Error?

* fix: remove react peerDep and add enhancements * fix: yarn versions * fix: linting * fix: add react and fluent as peerdeps * fix: deps and typings * fix: remove conditionals for dataproviders * fix: clean up controls boolean hook * fix: yarn install * fix: update version * fix: add changes to types in hooks and peerDeps * fix: linting * chore: add comments and disable linting for no hook-deps on singletons * fix: add console log on error handling Co-authored-by: Amber Hoak <amhoak@microsoft.com>

Amber Hoak added 3 commits November 24, 2020 14:31

fix: remove react peerDep and add enhancements

5eb63d6

fix: yarn versions

6c7d504

fix: linting

3495b26

ahoak requested a review from darthtrevino November 24, 2020 22:55

darthtrevino requested changes Nov 24, 2020

View reviewed changes

Amber Hoak added 4 commits November 24, 2020 15:29

fix: add react and fluent as peerdeps

a285ee8

fix: deps and typings

980e434

fix: remove conditionals for dataproviders

721f76e

fix: clean up controls boolean hook

a52880b

darthtrevino reviewed Nov 25, 2020

View reviewed changes

Amber Hoak added 2 commits November 24, 2020 16:48

fix: yarn install

759abf9

fix: update version

6350c96

ahoak changed the title ~~fix: remove react peerDep and add enhancements~~ fix: add enhancements #17, #12 Nov 25, 2020

ahoak changed the title ~~fix: add enhancements #17, #12~~ fix: add enhancements #17, #12 (custom settings) Nov 25, 2020

darthtrevino reviewed Nov 25, 2020

View reviewed changes

Amber Hoak added 2 commits November 25, 2020 10:01

fix: add changes to types in hooks and peerDeps

96d5b53

fix: linting

8d18b79

darthtrevino reviewed Nov 25, 2020

View reviewed changes

chore: add comments and disable linting for no hook-deps on singletons

5227fc8

darthtrevino reviewed Nov 30, 2020

View reviewed changes

darthtrevino approved these changes Nov 30, 2020

View reviewed changes

fix: add console log on error handling

8488dc2

ahoak merged commit 4aba0fd into main Nov 30, 2020

ahoak deleted the fix/hierarchy_browser branch November 30, 2020 17:22

This was referenced Dec 7, 2020

Unify Background Color for Neighbor Column Bars #17

Closed

Configurable columns in hierarchy browser tables #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add enhancements #17, #12 (custom settings) #24

fix: add enhancements #17, #12 (custom settings) #24

ahoak commented Nov 24, 2020 •

edited

darthtrevino Nov 24, 2020

darthtrevino Nov 24, 2020

darthtrevino Nov 24, 2020

darthtrevino Nov 24, 2020

darthtrevino Nov 24, 2020

darthtrevino Nov 25, 2020

ahoak Nov 25, 2020

darthtrevino Nov 25, 2020

darthtrevino left a comment

darthtrevino Nov 25, 2020

darthtrevino Nov 25, 2020

darthtrevino Nov 25, 2020

darthtrevino Nov 25, 2020

darthtrevino Nov 25, 2020

ahoak Nov 25, 2020 •

edited

darthtrevino Nov 25, 2020

ahoak Nov 25, 2020

darthtrevino Nov 25, 2020 •

edited

ahoak Nov 25, 2020

darthtrevino Nov 30, 2020

		() => new HierarchyDataProvider(communities, entities, neighbors),
		[],

fix: add enhancements #17, #12 (custom settings) #24

fix: add enhancements #17, #12 (custom settings) #24

Conversation

ahoak commented Nov 24, 2020 • edited

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

darthtrevino 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

ahoak Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthtrevino Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoak commented Nov 24, 2020 •

edited

ahoak Nov 25, 2020 •

edited

darthtrevino Nov 25, 2020 •

edited