wip: cleaning up the specs list and swapping it to be a filetree #15483
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
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 |
||||||||||||||||||||||||||
| white-space: nowrap; | ||
| } | ||
|
|
||
| .ul { |
There was a problem hiding this comment.
Just curios why classes are named like html tags, looks a little confusing as for me.
| @@ -0,0 +1,73 @@ | |||
| @use '../../index.scss' as *; | |||
There was a problem hiding this comment.
| @use '../../index.scss' as *; | |
| @import '../../index.scss'; |
There was a problem hiding this comment.
No. You want @use. It deduplicates imports and has many other benefits. https://css-tricks.com/introducing-sass-modules/#import-files-with-use
There was a problem hiding this comment.
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.
Why did we decide not to use css-in-js?
| ] | ||
|
|
||
| describe('FileExplorer', () => { | ||
| it('renders', () => { |
There was a problem hiding this comment.
Any assertions? At least cy.persySnapshot?
There was a problem hiding this comment.
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.
| updateItem(item, idx, { isOpen: !item.isOpen }) | ||
| } | ||
|
|
||
| return (<React.Fragment key={item.shortName}> |
There was a problem hiding this comment.
| return (<React.Fragment key={item.shortName}> | |
| return ( | |
| <React.Fragment key={item.shortName}> |
| import styles from './FileExplorer.module.scss' | ||
| import cs from 'classnames' | ||
|
|
||
| const icons: Record<string, any> = { |
There was a problem hiding this comment.
| const icons: Record<string, any> = { | |
| const icons: Record<string, { icon: typeof reactTs }> = { |
Or pass the right type
There was a problem hiding this comment.
Thanks. There's a lot I don't really understand how to do.
| folderClosed: { icon: folderClosed }, | ||
| } | ||
|
|
||
| const getExt = (path: string) => { |
There was a problem hiding this comment.
Why not to use https://nodejs.org/api/path.html#path_path_extname_path?
There was a problem hiding this comment.
Ohh that is frontend. Sorry :D
| } | ||
|
|
||
| return ( | ||
| <a |
There was a problem hiding this comment.
Please use <button for a11y reasons
There was a problem hiding this comment.
Why? Isn't a more correct because we're doing a hash navigation change?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| * Split specs into group by their | ||
| * first level of folder hierarchy | ||
| * | ||
| * @param {Array<{name: string}>} specs |
There was a problem hiding this comment.
It looks weird, because we have different type in the function declaration
| * @param {Array<{name: string}>} specs |
There was a problem hiding this comment.
Yeah this was copy/paste foul.
| export type FolderOrFile = File | Folder | ||
|
|
||
| export interface File extends FileLike { | ||
| type: 'file' |
|
|
||
| return props.selectedSpecs.includes(item.absolute) | ||
| } | ||
| }/> |
There was a problem hiding this comment.
| }/> | |
| } | |
| /> |
|
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 |
| isOpen: false, | ||
| }, | ||
| // @ts-ignore | ||
| { name: 'bar/foo.spec.tsx' }, |
There was a problem hiding this comment.
why do we need ts-ignore?
There was a problem hiding this comment.
This is wrong. It's because I was hacking on this file and hadn't finished the types.
| it('renders', () => { | ||
| mount(<> | ||
| <FileExplorer files={files} /> | ||
| </>) |
There was a problem hiding this comment.
Don't need fragment here
| return extensionMatches ? extensionMatches[1] : '' | ||
| } | ||
|
|
||
| export const FileExplorer: React.FC<FileExplorerProps> = (props) => { |
There was a problem hiding this comment.
spacing is quite confusing here, can we make it a bit cleaner
|
|
||
| export const FileExplorer: React.FC<FileExplorerProps> = (props) => { | ||
| 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.
Do we need all the || () => {}? Can we just make the props nullable?
There was a problem hiding this comment.
And even if not, you should use ??
There was a problem hiding this comment.
I don't think we can use ?? with our version of ts? I'm not sure.
There was a problem hiding this comment.
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.
?? 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
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
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.
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.
| style?: React.CSSProperties | ||
| getHref? (item: FolderOrFile): string | ||
| depth: number | ||
| onFocus: any |
| @@ -2,6 +2,8 @@ export * from './components/Button' | |||
|
|
|||
| export * from './components/CypressLogo/CypressLogo' | |||
|
|
|||
| export * from './components/FileExplorer/FileExplorer' | |||
There was a problem hiding this comment.
I think it's idiomatic to add a index.ts that exports everything. export * from './FileExplorer. Then this becomes export * from './components/FileExplorer'
There was a problem hiding this comment.
This was the pattern that @agg23 followed, so I did the same. I don't care one way or another. I've also seen the index.ts pattern and prefer it for convenience, but find it difficult when trying to search for things.
There was a problem hiding this comment.
I don't care right now. I imagine I will be changing around exports significantly when there starts to be atoms vs final components, so whatever is fine by me.
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. Having it decoupled from the model makes it easier to test and more portable because it's not reliant on the global stylesheets of runner-ct. Additionally, if we know we'll reuse the pattern, it'll be better to decouple from domain model + global styles early on before runner-ct's style leaks into the component. Once the component's styles are polluted, it's very hard and risky to split them out. |
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. |
|
This file explorer should not be a part of any shared library because it is too coupled with a context of runner-ct. Making it decoupled will be a pain I think. So I don't see any problems in putting it right in the runner-ct. Am I missing something? |
| onClick={(e) => { | ||
| props.onClick(e, props.item) | ||
| onClick(e, props.item) | ||
| }} |
There was a problem hiding this comment.
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}| { props.item.isSelected } | ||
| <li | ||
| style={{ ...props.style, ...inlineStyles.li }} | ||
| className={styles.li}> |
There was a problem hiding this comment.
| className={styles.li}> | |
| className={styles.li} | |
| > |
I mean... it works and it's easy to test, and will pay us forward if we use it for non-specs (workbench mode? stories? idk). I don't see a reason to revert it. In general, I don't really see a reason not to make most things decoupled from domain objects... if it's presentational, make it presentational 🤷🏻 Let's DM sometime, I'm curious as to why it's a good idea to couple. |
|
I am on board with @dmtrKovalenko regarding where this belongs. At least to me, a design system should be more generic. Things like buttons, lists, modals, etc. A spec list doesn't really fit into this in my opinion. What would belong here is a I think the way to make it decoupled would be something like <TreeView
leafComponent={Folder}
nodeComponent={File}
data={specHierarchy}
/>(Trivial example). It's similar to using Then you could re-use it for various other tree views (perhaps someone else needs a tree view, but does not want to have icons, etc). In Vue this is called a "renderless component". React often refers to this pattern is "render as a function", I think. So what we would need is
Anyway, I think we can discuss this further, for now I will push some simple changes to get this over the line and fix the tests. Regarding CSS modules vs CSS-in-JS, I don't have much experience around either, it's super annoying how you cannot see the class names in the DOM since CSS modules make them into a hash. I wonder if there is some flag to show something a bit more readable in the DOM. |
|
Closing in favor of #15513 |
This PR rewrites the DOM structure of the SpecList, as well as attempts to decouple the SpecList model from rendering the file tree.
I will need to pair with someone on this PR because it still needs a lot of work to remove technical debt before it is good to merge.
User facing changelog
Additional details
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?