feat(design-system): FileExplorer #15513

lmiller1990 · 2021-03-16T08:00:20Z

Hi! I'd like to show how I think we should approach building out our design system. I did not integrate the new <SpecList> to runner-ct yet, I want to get some feedback and opinions first. I think we should be careful how and what goes into the design system. Anything in their should be Cypress agnostic (see below).

This PR is based on the work in this PR but differs slightly. In #15483, the idea of a <SpecList> and a <FileExplorer> are one in the same. To get maximium reusability and value from our design system, I think the component in npm/design-system should not be Cypress specific.

This PR splits the previous <FileExplorer> into two separate components:

<FileExplorer>
<SpecList>

The primary difference is <FileExplorer> does not know about Cypress, Cypress.Cypress['spec'], or how it renders. Out of the box, it looks like this (no styling). The <File> and <Folder> components are provided via props (so the user has control over how they look).

To demonstrate I also added test (story?) with basic style like underlining the selected file. Styles are provided via props:

Notice FileExplorer does not contain any references to things like Cypress, Specs, etc.

Next, I built the <SpecList> component using the <FileExplorer>. You can easily customize using props (css, fileComponent, folderComponent, etc):

Finally, I don't think <SpecList> even belongs in the design system. Or, at least, it should be in a different part of the design system, which is scoped to Cypress specific components. Personally I don't see how or where this could be reused. We render a little React icon next to tsx files - E2E has no such concept as a "react" file, just general, framework agnostic spec files, so it wouldn't make sense to use there. For this reason I think <SpecList> belongs in packages/runner-ct, and the generic building block, <FileExplorer>, belongs in npm/design-system.

cypress-bot · 2021-03-16T08:00:22Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-03-16T08:09:00Z

Test summary

3984 • 0 • 49 • 2 • 0

Run details


Project	cypress
Status	Passed
Commit	`1d34aa7`
Started	Mar 17, 2021 7:17 AM
Ended	Mar 17, 2021 7:27 AM
Duration	09:59 💡
OS	Linux Debian - 10.5
Browser	Chrome 83

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

agg23

Generally fine for now, but there's many things I will likely be changing for future shared use.

agg23 · 2021-03-16T13:23:18Z

npm/design-system/src/components/FileExplorer/FileExplorer.spec.tsx

+    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>
+      )
+    }


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

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.

agg23 · 2021-03-16T13:25:32Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+  // 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
+  }


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?

Yeah just left over from the first round through.

agg23 · 2021-03-16T13:27:11Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+   */
+  const [openFolders, setOpenFolders] = React.useState<Record<string, boolean>>({})
+
+  React.useEffect(() => {


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

Neat, I did not know about useLayoutEffect. 🤦

agg23 · 2021-03-16T13:29:17Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

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


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

agg23 · 2021-03-16T13:32:28Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+  // 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})`,
+    },
+  }


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.

agg23 · 2021-03-16T13:53:24Z

npm/design-system/src/components/SpecList/SpecList.tsx

+
+  return (
+    <div onClick={() => props.onClick(props.item)}>
+      {<InlineIcon {...inlineIconProps} />}


This does not need to be in curly braces (I'm adding a lint rule for it :P )

npm/design-system/src/components/SpecList/SpecList.tsx

agg23 · 2021-03-16T13:56:57Z

npm/design-system/src/components/SpecList/SpecList.tsx

+interface SpecListProps extends Omit<
+  FileExplorerProps, 'files' | 'fileComponent' | 'folderComponent' | 'cssModule'
+> {
+  specs: Cypress.Cypress['spec'][]


It is unclear to me where these Cypress types are coming from. I'm guessing there's a global d.ts declaring them, but we should put an interim module be explicitly importing the types instead of assuming they exist.

It's a mystery to me, too, but they are available everywhere!

agg23 · 2021-03-16T13:57:28Z

npm/design-system/src/components/SpecList/SpecList.tsx

+}
+
+export const SpecList: React.FC<SpecListProps> = (props) => {
+  const files = makeFileHierarchy(props.specs.map((spec) => spec.relative))


This is a massive amount of work to perform in render. You want to wrap this in useMemo

agg23 · 2021-03-16T13:57:57Z

npm/design-system/tsconfig.json

@@ -5,7 +5,7 @@
    "module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */,
    "skipLibCheck": true,
    "lib": [
-      "es2015",
+      "es2016",


Do we have actual requirements here? Can we just go up to ESNext?

Older Chrome browsers

Array.includes!

…io/cypress into feat/spec-list-refactor

* wip: cleaning up the specs list and swapping it to be a filetree * wip: refactor * wip: refactor * wip: work on refactor * chore: improve types * styling * chore: remove all references to Cypress in tree list * chore: remove all references to Cypress in tree list * extract spec list component * write some tests * correctly update state * chore: refactor * update test * make props optional * optimizations * add back search * use memo * run spec * run spec * fix a11l nav * add tests for spec list a11y nav * fix tests: * remove unused * update version Co-authored-by: Jessica Sachs <jess@jessicasachs.io>

JessicaSachs and others added 14 commits March 15, 2021 06:48

wip: cleaning up the specs list and swapping it to be a filetree

f1a4901

Merge branch 'develop' into feat/spec-list-runner-ct

d1bc1ec

wip: refactor

032652a

wip: refactor

4032a76

wip: work on refactor

228fe88

chore: improve types

98c62f8

styling

e882438

chore: remove all references to Cypress in tree list

024ed22

chore: remove all references to Cypress in tree list

364f937

extract spec list component

43070d8

write some tests

7d8a2d3

correctly update state

24985b4

chore: refactor

bbae2dd

update test

c36aa44

lmiller1990 requested review from JessicaSachs, agg23, dmtrKovalenko and elevatebart March 16, 2021 08:01

lmiller1990 marked this pull request as draft March 16, 2021 08:20

make props optional

6eca2bf

agg23 previously approved these changes Mar 16, 2021

View reviewed changes

JessicaSachs marked this pull request as ready for review March 16, 2021 17:03

JessicaSachs mentioned this pull request Mar 16, 2021

wip: cleaning up the specs list and swapping it to be a filetree #15483

Closed

5 tasks

JessicaSachs and others added 4 commits March 16, 2021 17:41

Merge branch 'develop' into feat/spec-list-refactor

66fdee3

optimizations

ba6faa3

add back search

0713905

Merge branch 'feat/spec-list-refactor' of https://github.com/cypress-…

b44f46f

…io/cypress into feat/spec-list-refactor

lmiller1990 dismissed agg23’s stale review via b44f46f March 17, 2021 01:33

This was referenced Mar 18, 2021

Bump cypress from 6.2.1 to 6.8.0 sfbrigade/datasci-police-budget#276

Closed

build(deps-dev): bump cypress from 6.7.1 to 6.8.0 electron/electronjs.org-old#5239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(design-system): FileExplorer #15513

feat(design-system): FileExplorer #15513

lmiller1990 commented Mar 16, 2021 •

edited

Loading

cypress-bot bot commented Mar 16, 2021

cypress bot commented Mar 16, 2021 •

edited

Loading

agg23 left a comment

agg23 Mar 16, 2021

lmiller1990 Mar 17, 2021

agg23 Mar 16, 2021

JessicaSachs Mar 16, 2021

agg23 Mar 16, 2021

lmiller1990 Mar 17, 2021

lmiller1990 Mar 17, 2021

agg23 Mar 16, 2021

agg23 Mar 16, 2021

agg23 Mar 16, 2021

agg23 Mar 16, 2021

lmiller1990 Mar 16, 2021

agg23 Mar 16, 2021

lmiller1990 Mar 17, 2021

agg23 Mar 16, 2021

JessicaSachs Mar 16, 2021

lmiller1990 Mar 16, 2021

feat(design-system): FileExplorer #15513

feat(design-system): FileExplorer #15513

Conversation

lmiller1990 commented Mar 16, 2021 • edited Loading

cypress-bot bot commented Mar 16, 2021

cypress bot commented Mar 16, 2021 • edited Loading

Test summary

Run details

agg23 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

lmiller1990 commented Mar 16, 2021 •

edited

Loading

cypress bot commented Mar 16, 2021 •

edited

Loading