Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(design-system): FileExplorer #15513

Merged
merged 27 commits into from
Mar 17, 2021
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f1a4901
wip: cleaning up the specs list and swapping it to be a filetree
JessicaSachs Mar 15, 2021
d1bc1ec
Merge branch 'develop' into feat/spec-list-runner-ct
lmiller1990 Mar 15, 2021
032652a
wip: refactor
lmiller1990 Mar 16, 2021
4032a76
wip: refactor
lmiller1990 Mar 16, 2021
228fe88
wip: work on refactor
lmiller1990 Mar 16, 2021
98c62f8
chore: improve types
lmiller1990 Mar 16, 2021
e882438
styling
lmiller1990 Mar 16, 2021
024ed22
chore: remove all references to Cypress in tree list
lmiller1990 Mar 16, 2021
364f937
chore: remove all references to Cypress in tree list
lmiller1990 Mar 16, 2021
43070d8
extract spec list component
lmiller1990 Mar 16, 2021
7d8a2d3
write some tests
lmiller1990 Mar 16, 2021
24985b4
correctly update state
lmiller1990 Mar 16, 2021
bbae2dd
chore: refactor
lmiller1990 Mar 16, 2021
c36aa44
update test
lmiller1990 Mar 16, 2021
6eca2bf
make props optional
lmiller1990 Mar 16, 2021
66fdee3
Merge branch 'develop' into feat/spec-list-refactor
JessicaSachs Mar 16, 2021
ba6faa3
optimizations
lmiller1990 Mar 17, 2021
0713905
add back search
lmiller1990 Mar 17, 2021
b44f46f
Merge branch 'feat/spec-list-refactor' of https://github.com/cypress-…
lmiller1990 Mar 17, 2021
0dce853
use memo
lmiller1990 Mar 17, 2021
50a6525
run spec
lmiller1990 Mar 17, 2021
dd9f517
run spec
lmiller1990 Mar 17, 2021
5a9f421
fix a11l nav
lmiller1990 Mar 17, 2021
83beeac
add tests for spec list a11y nav
lmiller1990 Mar 17, 2021
4512aa7
fix tests:
lmiller1990 Mar 17, 2021
1a18b70
remove unused
lmiller1990 Mar 17, 2021
1d34aa7
update version
lmiller1990 Mar 17, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions npm/design-system/.mocharc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"watch-ignore": [
"./test/_test-output",
"node_modules"
],
"require": "ts-node/register",
"exit": true
}
3 changes: 3 additions & 0 deletions npm/design-system/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
"@fortawesome/free-brands-svg-icons": "5.15.2",
"@fortawesome/free-solid-svg-icons": "5.15.2",
"@fortawesome/react-fontawesome": "0.1.14",
"@iconify/icons-vscode-icons": "1.1.1",
"@iconify/react": "1.0.0",
"classnames": "2.2.6",
"debug": "4.3.2"
},
Expand All @@ -41,6 +43,7 @@
"babel-loader": "8.0.6",
"css-loader": "2.1.1",
"cypress": "0.0.0-development",
"mocha": "^7.0.1",
"react": "16.8.6",
"react-dom": "16.8.6",
"rollup": "^2.38.5",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
@use '../../index.scss' as *;

.nav {
user-select: none;
white-space: nowrap;
}

.ul {
margin-block-start: 0;
margin-block-end: 0;
margin-inline-start: 0;
margin-inline-end: 0;
padding-inline-start: 0;
&:before {
display: none;
}
}

.li.li {
padding-left: 20px;
}


.ul, .li {
position: relative;
list-style: none;
font-size: $text-s;
line-height: 1.6;
}

.a {
position: relative;
color: unset;
text-decoration: none;
display: inline-block;
width: 100%;
&:hover {
cursor: pointer;
}
}

.ul .ul {
margin-inline-start: $text-xs;
}

.isSelected, .isSelected:hover {
text-decoration: underline;
}
130 changes: 130 additions & 0 deletions npm/design-system/src/components/FileExplorer/FileExplorer.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import { mount } from '@cypress/react'
import React from 'react'
import { FileExplorer, FileComponentProps, FolderComponentProps } from './FileExplorer'
import { FileNode, makeFileHierarchy, TreeNode } from './helpers/makeFileHierarchy'

import styles from './FileExplorer.module.scss'

const specs: Cypress.Cypress['spec'][] = [
{
relative: 'foo/bar/foo.spec.js',
absolute: 'Users/code/foo/bar/foo.spec.js',
name: 'foo/bar/foo.spec.js',
},
{
relative: 'bar/foo.spec.tsx',
absolute: 'bar/foo.spec.tsx',
name: 'bar/foo.spec.tsx',
},
{
relative: 'merp/map.spec.ts',
absolute: 'merp/map.spec.ts',
name: 'merp/map.spec.ts',
},
]

interface FileExplorerTestProps {
clickFileStub: typeof cy.stub
clickFolderStub: typeof cy.stub
}

function createFileExplorer (testProps: FileExplorerTestProps): React.FC {
return () => {
const [selectedFile, setSelectedFile] = React.useState<string>()

const onFileClick = (file: FileNode) => {
setSelectedFile(file.absolute)
}

const files = makeFileHierarchy(specs.map((spec) => spec.relative))

const FileComponent: React.FC<FileComponentProps> = (props) => {
return (
<div onClick={() => {
testProps.clickFileStub(props.item)
props.onClick(props.item)
}}>
{props.item.name}
</div>
)
}

const FolderComponent: React.FC<FolderComponentProps> = (props) => {
return (
<div onClick={() => {
testProps.clickFolderStub()
props.onClick()
}}>
{props.item.name}
</div>
)
}
Comment on lines +41 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ideal (non-test) usage, these would be statically defined at the module level, rather than inline. Alternatively, they could be memoed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is just for testing purposes. We should make a new component in practice. I'll make this change in the other file where they are declared inline.


return (
<FileExplorer
files={files}
cssModule={styles}
selectedFile={selectedFile}
fileComponent={FileComponent}
folderComponent={FolderComponent}
onFileClick={onFileClick}
/>
)
}
}

describe('FileExplorer', () => {
it('basic usage', () => {
const files: TreeNode[] = [
{
type: 'folder',
name: 'foo',
absolute: 'foo',
files: [
{
type: 'file',
name: 'bar.js',
absolute: 'foo/bar.js',
},
],
},
]

const FileComponent: React.FC<FileComponentProps> = (props) => <div>{props.item.name}</div>
const FolderComponent: React.FC<FolderComponentProps> = (props) => <div>{props.item.name}</div>

mount(
<FileExplorer
files={files}
selectedFile={undefined}
fileComponent={FileComponent}
folderComponent={FolderComponent}
onFileClick={() => {}}
/>,
)
})

it('clicks file and folders', () => {
const clickFolderStub = cy.stub()
const clickFileStub = cy.stub()

const Wrapper = createFileExplorer({
clickFolderStub,
clickFileStub,
})

mount(<Wrapper />)

cy.get('div').contains('bar').click().then(() => {
expect(clickFolderStub).to.have.been.calledWith()
})

cy.get('div').contains('map.spec.ts').click().then(() => {
expect(clickFileStub).to.have.been.calledWith({
type: 'file',
absolute: 'merp/map.spec.ts',
name: 'map.spec.ts',
})
})
})
})
177 changes: 177 additions & 0 deletions npm/design-system/src/components/FileExplorer/FileExplorer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
import React from 'react'
import cs from 'classnames'
import { FileNode, FolderNode, TreeNode } from './helpers/makeFileHierarchy'

export interface FolderComponentProps {
item: FolderNode
depth: number
isOpen: boolean
onClick: () => void
}

export interface FileComponentProps {
item: FileNode
depth: number
onClick: (file: FileNode) => void
}

export interface FileExplorerProps extends React.HTMLAttributes<HTMLDivElement> {
files: TreeNode[]
fileComponent: React.FC<FileComponentProps>
folderComponent: React.FC<FolderComponentProps>
selectedFile?: string
onFileClick: (file: FileNode) => void

// Styles. They should be a *.module.scss.
// TODO: Can we type these? Do we want to couple to CSS modules?
cssModule?: {
nav: any
ul: any
li: any
a: any
isSelected: any
}
Comment on lines +28 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if this is from Jess's work, but this is quite an unusual pattern. It's fine if we actually need to customize the styles of every little thing, but why would we need to? Additionally, why are these not contextual names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just left over from the first round through.

}

export interface FileTreeProps extends FileExplorerProps {
depth: number
openFolders: Record<string, boolean>
style?: React.CSSProperties
setSelectedFile: (absolute: string) => void
}

export const FileExplorer: React.FC<FileExplorerProps> = (props) => {
/**
* Whether a folder is open or not is a **UI** concern.
* From a file system point of view, there is no such concept as "open" or "closed",
* only from a user's point of view.
* For this reason we save the open state as part of the UI component. The easiest
* way to do this is a key/value pair, mapping the absolute path of a directory to a boolean
*
* {
* 'foo': true,
* 'foo/bar': true
* 'foo/bar/qux': false
* }
*
* Every directory is set to open by default. When you add a new directory
* or file via your file system (eg mkdir foo/bar && touch foo/bar/hello.js) it will be added
* without losing the current state of open/closed directories.
*/
const [openFolders, setOpenFolders] = React.useState<Record<string, boolean>>({})

React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a useLayoutEffect as you need these changes to occur before the render is committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, I did not know about useLayoutEffect. 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

function walk (nodes: TreeNode[]) {
for (const node of nodes) {
if (node.type === 'folder') {
if (!(node.absolute in openFolders)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want this to not override previously collapsed folders (which is what this does), you should probably call it out in a comment.

setOpenFolders({ ...openFolders, [node.absolute]: true })
}

walk(node.files)
}
}
}

walk(props.files)
}, [props.files, openFolders])

const setSelectedFile = (absolute: string) => {
setOpenFolders({ ...openFolders, [absolute]: !openFolders[absolute] })
}

return (
<nav className={cs(props.className, props.cssModule.nav)}>
<FileTree
{...props}
setSelectedFile={setSelectedFile}
openFolders={openFolders}
depth={0}
/>
</nav>
)
}

export const FileTree: React.FC<FileTreeProps> = (props) => {
// Negative margins let the <a> tag take full width (a11y)
// while the <li> tag with text content can be positioned relatively
// This gives us HTML + cssModule-only highlight and click handling
const inlineStyles = {
a: {
marginLeft: `calc(-20px * ${props.depth})`,
width: `calc(100% + (20px * ${props.depth}))`,
},
li: {
marginLeft: `calc(20px * ${props.depth})`,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only a problem because you're recursively defining the tree and childing the DOM elements in correlation to the tree nesting. This is fine for now, but I will be replacing it with a flat (and virtualized) list for performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm mistaken, both of these fields are only consumed once. Why are they not defined inline?

Also, please actually calculate the positions in advance if possible. No need to use calc in situations where we know the sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done... we still have one calc, but it'll do for now until we replace it with something more optimize.


const fileTree = (item: TreeNode) => {
if (item.type !== 'folder') {
return
}

return (
<FileTree
fileComponent={props.fileComponent}
folderComponent={props.folderComponent}
openFolders={props.openFolders}
setSelectedFile={props.setSelectedFile}
onFileClick={props.onFileClick}
selectedFile={props.selectedFile}
depth={props.depth + 1}
cssModule={props.cssModule}
files={props.openFolders[item.absolute] ? item.files : []}
/>
)
}

const renderFolder = (item: FolderNode) => {
return (
<props.folderComponent
depth={props.depth}
item={item}
isOpen={props.openFolders[item.absolute]}
onClick={() => props.setSelectedFile(item.absolute)}
/>
)
}

const renderFile = (item: FileNode) => {
return (
<props.fileComponent
depth={props.depth}
item={item}
onClick={props.onFileClick}
/>
)
}
Comment on lines +167 to +206
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stuff you really want to prevent in render, as it adds additional cost due to the dynamic nature of your JSX. It would be better if these were statically defined components, and those components received all of the props necessary.

This particular situation isn't as bad because you're only adding new JSX nodes with the functions, however, if you were using these to create callbacks, there would be a real performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, my goal here was to let the user provide their own implementation for the file/folders. Is there some other pattern to accomplish this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make them entirely static, you can make your props take either JSX.Element, or an actual React component (React.Component<YourPropType>). With JSX.Element, you would pass just a normal JSX component invocation to your prop. This won't work since you want to bind to your item (well, you can make it work, but I won't go into that).

Instead, you could define your prop interface, then pass a React component that receives all of those props and does something with it. In that scenario you'd be passing the actual React function (or class) instance.

Finally, if you do want to do "render props" (which is essentially what you've done), you probably want to wrap the functions in useCallback.

I want to emphasize that with how you have written this, I don't think any change is necessary, and the first two suggestions would only bring extremely minor performance fixes. I was more just commenting for learning purposes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for renderFile and other render props to be passed directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, good to know. Thanks for the feedback.


return (
<ul className={props.cssModule.ul}>
{
props.files.map((item) => {
return (
<React.Fragment key={item.absolute}>
<a
style={inlineStyles.a}
className={cs(props.cssModule.a, {
[props.cssModule.isSelected]: props.selectedFile === item.absolute,
})}
tabIndex={0}
>
<li
style={{ ...props.style, ...inlineStyles.li }}
className={props.cssModule.li}>
{item.type === 'folder' && renderFolder(item)}
{item.type === 'file' && renderFile(item)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item.type === 'folder' ? renderFolder(item) : renderFile(item)

</li>
</a>
{fileTree(item)}
</React.Fragment>
)
})
}
</ul>
)
}
Loading