-
Notifications
You must be signed in to change notification settings - Fork 59
feat(ContentNavigator): added more detailed path information #1597
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hey @kpatl1! Thanks for taking a look at this. Only a couple things I noticed...
- If you hover over
SAS Server
, it'll show a tooltip that isn't quite right. We could honestly probably update the implementation inRestServerAdapter.getPathOfItem
andItcServerAdapter.getPathOfItem
to return an empty string for the root folder - Currently, the recycle bin has a path tooltip. Is this expected (is that a valid path)?
- Last, the only concern I have is that this will be called once per file or folder. So if we have a folder w/ 50 files, that's 51 requests to load the files (looks like this is only an issue w/ sas content). Have we tested how this impacts things if we have lots of files (i.e. btw 50-100 files)? The good thing is vscode does a decent job at caching these items so we shouldn't call it over and over. This is probably a non-issue, but just wanted to call it out for resting
Thinking about it further, you can just check if the path can be copied before you show the tooltip (i.e. checking |
…only fetch valid paths
I, Kishan Patel <[email protected]>, hereby add my Signed-off-by to this commit: c0622a9 I, Kishan Patel <[email protected]>, hereby add my Signed-off-by to this commit: 64965fe Signed-off-by: Kishan Patel <[email protected]>
You're totally right. Thank you for pointing these things out to me. I have gone in and added the copyPath optimization to only show valid paths. Furthermore, I have added caching for the parent to allow for the path to be concatenated for every sibling after the original server call. I am not sure if I fully implemented it correctly, so I would love if you could take a look at it when you get a chance. |
I have the same concern with @scottdover about the network call. It's not only one call per file but n calls per file where n is the nested level, since the |
|
||
if (!this.parentPathCache.has(parentUri)) { | ||
try { | ||
const fullPath = await this.model.getPathOfItem(item); |
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.
It may also be worth updating RestContentAdapter.getPathOfItem
to handle its own caching since it's really the only ContentAdapter
that requires it. And doing that w/n RestContentAdapter may make things easier w/r/t Wei's comment about using uri
…more efficent path retrieval
I, Kishan Patel <[email protected]>, hereby add my Signed-off-by to this commit: 05fb7f2 Signed-off-by: Kishan Patel <[email protected]>
Made a new commit by changing the uri to contain the full path. I have also moved the caching into the RestContentAdapter. I cannot seem to get the content in SAS Server to show the full path, however, only when I view items in My Folder. Do you guys have any advice on how I could approach this? |
Summary
This change improves the user experience in the SAS Content Navigator by showing full hierarchical paths when hovering over files and folders, instead of just displaying the filename.
Key Changes:
ContentDataProvider.getTreeItem()
to fetch and display full path information in tooltipsBefore:
/test1.sas
After:
/My Folder/Subfolder/test1.sas
Testing
-Verified tooltips show full paths for files in nested folder structures
Fixes #1577