Skip to content

fix(site): Show folders in the template version editor #6145

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

Merged
merged 9 commits into from
Feb 10, 2023

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Feb 9, 2023

A quick video review:
https://www.loom.com/share/7297f453b65a47388fd51e9820af53b3

PS: It does not fix the version update missing files.

@BrunoQuaresma BrunoQuaresma requested review from kylecarbs and a team February 9, 2023 20:56
@BrunoQuaresma BrunoQuaresma self-assigned this Feb 9, 2023
@BrunoQuaresma BrunoQuaresma requested review from code-asher and removed request for a team February 9, 2023 20:56
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

One quick thought I had (have not thoroughly reviewed everything) is that I think the typeof object checks are a little obscure. Could we do similar to Node's fs.stat response and have an isDirectory on a File interface? So maybe something like (probably not exactly this):

export interface File {
  path: string
  isDirectory: boolean
}

Edit: or we could have children and if there are none we assume it is a directory although I think that is still slightly obtuse. Maybe we have an isDirectory() which is implemented as typeof this.children !== "undefined". It could also make sense to have separate interfaces for files and directories, idk if that is too overengineered though

@BrunoQuaresma
Copy link
Collaborator Author

@code-asher I understand all the concerns but it would change all the implementation since it is based in a very simple tree structure. If you think it is something we definitely should do, I would say for us doing that after merge this PR to not block the fix.

@code-asher
Copy link
Member

I do not feel strongly about it, I just stumbled over the object checks at first as it was not clear what was happening until I read the rest of the file tree code. Feel free to ignore!

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I have not actually tested it because for the life of me I have not been able to figure out how to get the template editor. 😛 You probably already noticed the Storybook errors but just in case you have not seen them looks like some stories need to be updated.

@code-asher
Copy link
Member

code-asher commented Feb 9, 2023

Forgot to say I like the tree structure and I see now how the current interface makes it easy to use things like lodash.has. I understand now it would be unfortunate to lose that convenience if we were to change the interface to add an isDirectory since we would have to recurse manually. Maybe in the future we wrap that object check in a isFileTree(t: FileTree | string): t is FileTree convenience function just for clarity but keep the current interface and implementation.

@code-asher
Copy link
Member

code-asher commented Feb 10, 2023

I tried to manually go to the view and edit template pages to test them out but I keep getting error loading dynamically imported module. Probably something I am doing wrong, but just a heads up in case this is not me.

@BrunoQuaresma
Copy link
Collaborator Author

This should not be failing tho. Let me see if I can find any issue.

@BrunoQuaresma BrunoQuaresma merged commit 77afdf7 into main Feb 10, 2023
@BrunoQuaresma BrunoQuaresma deleted the bq/ui-tweaks-editor branch February 10, 2023 16:22
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants