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

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Mar 16, 2021

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:

  1. <FileExplorer>
  2. <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).

image

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

image

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):

image

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
Copy link
Contributor

cypress-bot bot commented Mar 16, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 16, 2021



Test summary

3984 0 49 2Flakiness 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

@lmiller1990 lmiller1990 marked this pull request as draft March 16, 2021 08:20
agg23
agg23 previously approved these changes Mar 16, 2021
Copy link
Contributor

@agg23 agg23 left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +41 to +61
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>
)
}
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.

Comment on lines +25 to +33
// 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
}
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.

*/
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.

Comment on lines 96 to 107
// 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.


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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

export const SpecList: React.FC<SpecListProps> = (props) => {
const files = makeFileHierarchy(props.specs.map((spec) => spec.relative))
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 a massive amount of work to perform in render. You want to wrap this in useMemo

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Older Chrome browsers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Array.includes!

This was referenced Mar 18, 2021
elevatebart pushed a commit that referenced this pull request Mar 18, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants