-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
@use '../../index.scss' as *; | ||
|
||
.nav { | ||
user-select: none; | ||
white-space: nowrap; | ||
} | ||
|
||
.ul { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need ts-ignore? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any assertions? At least cy.persySnapshot? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} /> | ||
</>) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need fragment here |
||
}) | ||
}) |
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) => { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>) | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need all the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And even if not, you should use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://babeljs.io/docs/en/babel-plugin-proposal-optional-chaining if not There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 || []) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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}> | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
<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> | ||||||||
) | ||||||||
} |
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> = { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Or pass the right type There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not to use https://nodejs.org/api/path.html#path_path_extname_path? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are passing both So if you want to use url – you need to subscribe to url changes and use hash without any internal state There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}> | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
{<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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
}, []) | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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-useThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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?