-
Notifications
You must be signed in to change notification settings - Fork 323
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
Expand parent directories of opened projects #11947
base: develop
Are you sure you want to change the base?
Conversation
🧪 Storybook is successfully deployed!📊 Dashboard:
|
queryClient, | ||
remoteBackend, | ||
setExpandedDirectoryIds, | ||
user.userGroups, |
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.
@MrFlashAccount not too sure about this one. should i be putting things in a ref like elsewhere, to make sure it doesn't unintentionally open all parent directories out of nowhere?
(is this not the right approach at all?)
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.
I don't get why it's an useEffect
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.
true, what's the alternative though? i think we need to re-expand the directories every time the category changes, right?
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.
we can avoid resetting expandedDirectories
every time the category changes and only run this one. but i think it'd still need to be a useEffect
, right? i don't think it should be hardcoded to be permanently expanded because otherwise you wouldn't collapse directories (e.g. if the directory has a lot of children so you want to collapse it)
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.
actually, i don't know if this is an option right now as currently we depend on expandedDirectoryIds
to control which listDirectory
queries are being fetched
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.
yup. that was my original concern, but i don't see a way to avoid it fully. in the react compiler optimization PR i just moved dependencies that aren't real dependencies to a syncref but if you have a better idea i'm definitely down to change it
We have useEventCallback
for that, here's a good explanation on why it's needed: https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md
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.
sounds good, but i'm worried about discoverability. i think it'd be cool for the icons to change into buttons to trigger dropdown menus on hover, but i dunno whether that's a good idea
Yeah, good idea
Though, we need to add ActionMenu component first: https://react-spectrum.adobe.com/react-spectrum/ActionMenu.html#actionmenu
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.
rfe: useEventCallback... thoughts? do you mean to reference non-deps in the eventCallback directly and then use a useEffect
that passes the reactive deps into the function?
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.
One thing on implementation: ideally this should support multitabs
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.
thoughts? do you mean to reference non-deps in the eventCallback directly and then use a useEffect that passes the reactive deps into the function?
Yeah, like that. Deps that shouldn't trigger effect, should be incapsulated inside the useEventCallback
function
Pull Request Description
Important Notes
None
Testing Instructions
enso/
to local favorites and open project)Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.
If applicable, it is suggested to paste a link to a successful run of the Extra Tests.