-
Notifications
You must be signed in to change notification settings - Fork 0
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
task/WP-65-DropdownViewFullPath #866
Conversation
Codecov Report
@@ Coverage Diff @@
## main #866 +/- ##
==========================================
+ Coverage 63.38% 63.40% +0.01%
==========================================
Files 430 432 +2
Lines 12297 12381 +84
Branches 2542 2574 +32
==========================================
+ Hits 7795 7850 +55
- Misses 4293 4319 +26
- Partials 209 212 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
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.
Looks functional. But there is room for improvement to the code.
client/src/components/DataFiles/DataFilesModals/DataFilesShowPathModal.jsx
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.scss
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.scss
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Show resolved
Hide resolved
…e-Portal into task/WP-65-DropdownViewFullPath
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.
Looking great so far! Functionality is working perfectly. A few comments on styling
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesModals/DataFilesShowPathModal.jsx
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
I made the Dropdown a separate component from DataFilesBreadcrumbs. Moved the Copy button in the modals from the left to the right (that was the original mock, and it looks better with everything left aligned in the modal imo). Also had to change the dropdown navigation logic from the previous commit. I tested it on Public and Community Data, but will test it on Frontera and other file systems tomorrow. Update: New navigation logic working correctly on Frontera and other systems too. |
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.
Working great for the most part! Found a few things worth discussing
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Outdated
Show resolved
Hide resolved
<div className={`breadcrumbs ${className}`}> | ||
{scheme === 'projects' && ( | ||
<> | ||
<RootProjectsLink | ||
api={api} | ||
section={section} | ||
operation={operation} | ||
label="Shared Workspaces" | ||
/>{' '} | ||
{system && `/ `} | ||
</> |
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.
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.
Looks good!
client/src/components/DataFiles/DataFilesBreadcrumbs/DataFilesBreadcrumbs.jsx
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesModals/DataFilesShowPathModal.jsx
Show resolved
Hide resolved
client/src/components/DataFiles/DataFilesDropdown/DataFilesDropdown.jsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
.storage-values, | ||
.storage-google { |
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 think we can just use storage-values
and get rid of storage-google
, since they have the same css
@@ -91,6 +100,7 @@ const DataFilesSelectModal = ({ isOpen, toggle, onSelect }) => { | |||
/> | |||
)} | |||
</div> | |||
<DataFilesShowPathModal api={apiValue} /> |
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.
Rather than define a new prop and initiate the dispatch here, can we move it to DataFilesShowPathModal
? If not, then we can instead use a selector to grab the api value in the DataFilesShowPathModal
component
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.
Getting some console errors when running locally. Was going to pass them on here in the review, but Tapis server also keeps intermittently going down making it difficult to test. May have to defer to the other reviewers for now.
Tapis came back up. Tested the issues previously brought up. LGTM good work! |
Looks great! Going to deploy to dev.cep to confirm that Google Drive works as well |
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.
Works and looks great! Left a couple comments, but not blockers to merge
client/src/components/DataFiles/DataFilesModals/DataFilesShowPathModal.jsx
Outdated
Show resolved
Hide resolved
dispatch({ | ||
type: 'FETCH_SYSTEM_DEFINITION', | ||
payload: system.system, | ||
}); |
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.
Was this needed both here and in DataFilesShowPathModal.jsx
? Just curious
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.
The idea/logic was every time the user selects a different system in the dropdown, it fetches that system definition (so the definition isn't null or undefined). ShowPathModal's dispatch would just be upon the modal opening, and since the modal is already open--my thought was it wouldn't fetch the new system definition without it also being in SystemSelector.
…athModal.jsx Co-authored-by: Sal Tijerina <[email protected]>
Overview
Started out as:
Some paths in the data files area can get quite long, and the breadcrumbs are often truncated. It may be ideal to offer a tooltip on hover to show the full path, untruncated.
Reach out to #cep-design for direction of design and functionality (and desirability of this feature).
I reached out to the cep-design team and Tracy for guidance on the design and functionality. The functionality changed from the original overview/goals. Instead of breadcrumb links, we now have a dropdown menu that contains the previous directories and the root directory. The current directory the user is in, is displayed to the right of the dropdown menu. Since WP-278 and WP-279 were related and basically required to complete WP-65, I have added them to this PR. We now have a new modal design for both the new "View Full Path" button/link and the "View Path" button/link in the File listings.
Related
Changes
Testing
UI
New Dropdown button and View Full Path Layout
Dropdown open
Truncate middle for long folder/file names
New Shared Workspaces dropdown layout with Project Name included for nav
New View Full Path Modal
Copy Modal with View Full Path
Move Modal with View Full Path
Select Modal with View Full Path
Notes