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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 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 Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
@use '../../index.scss' as *;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@use '../../index.scss' as *;
@import '../../index.scss';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. You want @use. It deduplicates imports and has many other benefits. https://css-tricks.com/introducing-sass-modules/#import-files-with-use

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, how much pain in the ass just to avoid css-in-js :D
I will never understand this probably

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we decide not to use css-in-js?


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

.ul {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curios why classes are named like html tags, looks a little confusing as for me.

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;
padding: 0;
line-height: 1.6;
}

.a {
position: relative;
color: unset;
text-decoration: none;
display: inline-block;
width: 100%;
&:hover {
background: opacify($color: $metal-05, $amount: 0.2);
}
}

.ul .ul::before {
content: "";
position: absolute;
z-index: 0;
width: 3px;
bottom: 0;
top: 0;
left: calc(1 * (#{$text-xs}));
background-size: 3px 3px;
background-image: radial-gradient(#999999 30%, transparent 0);
background-position: center;
display: block;
}

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

.folderIcon, .brandIcon {
margin-right: 0.5em;
}

.isClosed {
+ .ul, + .li {
display: none;
}
}

.isSelected, .isSelected:hover {
background: $chill-10;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { mount } from '@cypress/react'
import React from 'react'
import { FileExplorer } from './FileExplorer'
import { FileLike } from './types'

const files: FileLike[] = [
{
name: 'foo/bar/foo.spec.js',
onClick: (e, foo) => {
},
isOpen: false,
},
// @ts-ignore
{ name: 'bar/foo.spec.tsx' },
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need ts-ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is wrong. It's because I was hacking on this file and hadn't finished the types.

// @ts-ignore
{ name: 'merp/foo.spec.ts' },
]

describe('FileExplorer', () => {
it('renders', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any assertions? At least cy.persySnapshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I was using it as a playground, not for specs. It still needs specs... just was trying to get something up for the src to be reviewable.

mount(<>
<FileExplorer files={files} />
</>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need fragment here

})
})
114 changes: 114 additions & 0 deletions npm/design-system/src/components/FileExplorer/FileExplorer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React from 'react'
import { makeFileHierarchy } from './helpers/makeFileHierarchy'
import { FileExplorerProps, FileTreeProps, FileLike, FolderOrFile } from './types'
import styles from './FileExplorer.module.scss'
import { InlineIcon } from '@iconify/react'
import javascriptIcon from '@iconify/icons-vscode-icons/file-type-js-official'
import typescriptIcon from '@iconify/icons-vscode-icons/file-type-typescript-official'
import reactJs from '@iconify/icons-vscode-icons/file-type-reactjs'
import reactTs from '@iconify/icons-vscode-icons/file-type-reactts'
import folderClosed from '@iconify/icons-vscode-icons/default-folder'
import folderOpen from '@iconify/icons-vscode-icons/default-folder-opened'
import { library } from '@fortawesome/fontawesome-svg-core'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import { fas } from '@fortawesome/free-solid-svg-icons'
import cs from 'classnames'

library.add(fas)

const icons: Record<string, any> = {
js: { icon: javascriptIcon },
ts: { icon: typescriptIcon },
tsx: { icon: reactTs },
jsx: { icon: reactJs },
folderOpen: { icon: folderOpen },
folderClosed: { icon: folderClosed },
}

const getExt = (path: string) => {
const extensionMatches = path.match(/(?:\.([^.]+))?$/)

return extensionMatches ? extensionMatches[1] : ''
}

export const FileExplorer: React.FC<FileExplorerProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing is quite confusing here, can we make it a bit cleaner

return (<nav className={cs(props.className || '', styles.nav)}><FileTree files={makeFileHierarchy(props.files) || []} isSelected={props.isSelected || (() => {})} onClick={props.onClick || (() => {})}></FileTree></nav>)
}
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 need all the || () => {}? Can we just make the props nullable?

Copy link
Contributor

Choose a reason for hiding this comment

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

And even if not, you should use ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can use ?? with our version of ts? I'm not sure.

Copy link
Contributor

@dmtrKovalenko dmtrKovalenko Mar 15, 2021

Choose a reason for hiding this comment

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

We are not using ts for compilation it is extremely bad for this (if we are we should stop). Babel will handle this much better. ?? is a stage 3 so preset-env will be enough and I think it will work out of the box

Copy link
Contributor

Choose a reason for hiding this comment

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

?? is a stage 3 so preset-env will be enough and I think it will work out of the box

https://babeljs.io/docs/en/babel-plugin-proposal-optional-chaining if not

Copy link
Contributor

Choose a reason for hiding this comment

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

However, @dmtrKovalenko, if the TS version we're using doesn't support it, we'll get errors from the LSP


export const FileTree: React.FC<FileTreeProps> = (props) => {
const depth = props.depth || 0
const [files, setFiles] = React.useState(props.files || [])
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems incorrect - now we have two sources of truth. Why do we need two sources of truth? Can't we just use props.files? Design system should not have internal state, ideally everything should be a function of props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I be passing a callback all the way down and making the top-level component mutate files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it is weird that components like FileTree are inside the design system. Cause it looks like more "end" component, maybe better place will be inside runner-ct?

I think it makes sense because we will use it in runner-ct as well as with the node UI (possibly desktop-gui?). There's also some other benefits.

Technically we should have separate design system and Cypress component library folders (where the only components the design system has are individual atoms), but I don't know that it's worth going to that extreme now.

Yes. You're correct. I'd be happy just to get our design tokens plumbed first and also keep reusable UI patterns separate from domain models.


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

const updateItem = (i: FolderOrFile, idx: number, prop: Partial<FileLike>) => {
files[idx] = { ...i, ...prop }
setFiles([...files])
}

return (
<ul className={styles.ul}>
{
files.map((item, idx) => {
const ext = getExt(item.shortName) || ''
const folderIcon = item.isOpen ? icons.folderOpen : icons.folderClosed
const inlineIconProps = ext ? icons[ext] : folderIcon

function handleOnSelect (e: any) {
const onItemClick = item.onClick || (() => {})

onItemClick(e, item)
props.onClick(item)
updateItem(item, idx, { isOpen: !item.isOpen })
}

return (<React.Fragment key={item.shortName}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (<React.Fragment key={item.shortName}>
return (
<React.Fragment key={item.shortName}>

<a
data-file={item.relative}
key={item.shortName}
style={inlineStyles.a}
className={cs(styles.a, { [styles.isClosed]: !item.isOpen, [styles.isSelected]: props.isSelected(item) })}
onKeyDown={(e) => e.key === ' ' || e.key === 'Enter' && handleOnSelect(e)}
onClick={handleOnSelect}
tabIndex={0}>
<li
style={{ ...props.style, ...inlineStyles.li }}
className={styles.li}>
{ item.type === 'folder' &&
<FontAwesomeIcon
className={styles.folderIcon}
icon='chevron-up'
size='xs'
transform={{ rotate: item.isOpen ? 180 : 90 }} />
}

{<InlineIcon {...inlineIconProps} className={styles.brandIcon} />}
{ item.shortName }
</li>
</a>
{
item.type === 'folder' &&
<FileTree
depth={depth + 1}
files={item.files}
onClick={props.onClick}
isSelected={props.isSelected}
/>
}
</React.Fragment>)
})
}
</ul>
)
}
83 changes: 83 additions & 0 deletions npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import React, { ReactNode } from 'react'
import { InlineIcon } from '@iconify/react'
import javascriptIcon from '@iconify/icons-vscode-icons/file-type-js-official'
import typescriptIcon from '@iconify/icons-vscode-icons/file-type-typescript-official'
import reactJs from '@iconify/icons-vscode-icons/file-type-reactjs'
import reactTs from '@iconify/icons-vscode-icons/file-type-reactts'
import folderClosed from '@iconify/icons-vscode-icons/default-folder'
import folderOpen from '@iconify/icons-vscode-icons/default-folder-opened'
import { FolderOrFile } from './types'
import styles from './FileExplorer.module.scss'
import cs from 'classnames'

const icons: Record<string, any> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const icons: Record<string, any> = {
const icons: Record<string, { icon: typeof reactTs }> = {

Or pass the right type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. There's a lot I don't really understand how to do.

js: { icon: javascriptIcon },
ts: { icon: typescriptIcon },
tsx: { icon: reactTs },
jsx: { icon: reactJs },
folderOpen: { icon: folderOpen },
folderClosed: { icon: folderClosed },
}

const getExt = (path: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh that is frontend. Sorry :D

const extensionMatches = path.match(/(?:\.([^.]+))?$/)

return extensionMatches ? extensionMatches[1] : ''
}

export interface FileExplorerItemProps {
item: FolderOrFile
children?: ReactNode
style?: React.CSSProperties
getHref? (item: FolderOrFile): string
depth: number
onFocus: any
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we type these?

onClick: any
onBlur: any
}

export const FileExplorerItem: React.FC<FileExplorerItemProps> = (props) => {
const ext = getExt(props.item.shortName) || ''
const folderIcon = props.item.isOpen ? icons.folderOpen : icons.folderClosed
const getHref = props.getHref || (() => '#')
const onClick = props.item.onClick || (() => {})

const inlineIconProps = ext ? icons[ext] : folderIcon

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

return (
<a
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use <button for a11y reasons

Copy link
Contributor Author

@JessicaSachs JessicaSachs Mar 15, 2021

Choose a reason for hiding this comment

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

Why? Isn't a more correct because we're doing a hash navigation change?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are passing both onClick and href which is really weird. I don't understand this – it looks like you are trying to sync url and state which is a problem all the time. We need to have a single source of trust.

So if you want to use url – you need to subscribe to url changes and use hash without any internal state

Copy link
Contributor

Choose a reason for hiding this comment

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

href is fine as long as onClick is calling preventDefault. This is the "correct" way to build buttons that perform as links, so that you still get standard browser right/middle click behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it is unclear that it is doing prevent default. Maybe not, how would you know from this code? If it is – we need some kind of <Link /> component. But we don't really have any kind of routing, so it looks confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

style={inlineStyles.a}
className={cs(styles.a, { [styles.isClosed]: !props.item.isOpen, [styles.isSelected]: props.item.isSelected })}
href={getHref(props.item)}
onFocus={(e) => {
props.onFocus(e, props.item)
}}
onBlur={(e) => {
props.onBlur(e, props.item)
}}
onClick={(e) => {
props.onClick(e, props.item)
onClick(e, props.item)
}}
Comment on lines +71 to +74
Copy link
Contributor

@dmtrKovalenko dmtrKovalenko Mar 15, 2021

Choose a reason for hiding this comment

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

We can move it to function for readability

function handleClick(e: React.MouseEvent<HtmlLinkElement>) { 
   props.onClick(e, props.item);
   
   if (props.item.onClick) {
      props.item.onClick(e, props.item)
   }
}

onClick={handleClick}

tabIndex={0}>
{ props.item.isSelected }
<li
style={{ ...props.style, ...inlineStyles.li }}
className={styles.li}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className={styles.li}>
className={styles.li}
>

{<InlineIcon {...inlineIconProps} />}
{ props.children }
</li></a>)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { FileLike, Folder, FolderOrFile } from '../types'

/**
* Split specs into group by their
* first level of folder hierarchy
*
* @param {Array<{name: string}>} specs
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks weird, because we have different type in the function declaration

Suggested change
* @param {Array<{name: string}>} specs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was copy/paste foul.

*/
export function makeFileHierarchy (files: FileLike[]) {
// to save the existing folder paths
const kvpGroups: { [fullPath: string]: Folder } = {}

return files.reduce<FolderOrFile[]>((groups, file) => {
const pathArray = file.name.split('/')
let currentFileArray: FolderOrFile[] = groups
let currentPath = ''

do {
const pathPart = pathArray.shift() || ''

currentPath += `/${pathPart}`
// if we have a file set is as part of the current group
if (!pathArray.length) {
currentFileArray.push({
...file,
type: 'file',
shortName: pathPart,
currentPath,
})
} else if (pathPart) {
//if it is a folder find if the next folder exists
let currentGroup: Folder = kvpGroups[currentPath]

if (!currentGroup) {
//if it does not exist we create it
currentGroup = {
type: 'folder',
shortName: pathPart,
files: [],
isOpen: true,
name: pathPart,
currentPath,
}

kvpGroups[currentPath] = currentGroup
// and add it to the current set of objects
currentFileArray.push(currentGroup)
}

currentFileArray = currentGroup.files
}
} while (pathArray.length)

return groups
}, [])
}
Loading