[DT-789] Add Access Type Icons #2728
Conversation
cinyecai
requested review from
pshapiro4broad and
fboulnois
and removed request for
a team
|
I think from the mocks, the same icons need to be in the filter panel as well:
Additionally, in the case where there are multiple access types, the column should show a "Mixed" value:
|
@rushtong Are there datasets that have multiple access types? I thought the "Mixed" value would only apply to studies with datasets that have different access types |
@cinyecai - you are 100% correct, sorry about that! |
|
Added the icons to the filter list: |
| { filterName === 'Open' ? <div style={{display: 'flex', alignItems: 'center'}}> | ||
| <img src={openAccessIcon} style={{width: '10px', marginRight: 6}} /> {filterName} </div> : |
There was a problem hiding this comment.
Instead of using a nested ternary op, can this be done using a switch statement? Since all the other attributes are the same, the only thing that would change is the src attribute. The default case would either have to map to a placeholder icon (maybe a question mark since it's unexpected) or the external link icon.
There was a problem hiding this comment.
The images could use alt tags here.
| @@ -42,6 +45,38 @@ export const FilterItemList = (props) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export const AccessManagementFilterItemList = (props) => { | |||
There was a problem hiding this comment.
Instead of defining a new function, is there a way to combine this with FilterItemList? If an additional prop is an optional function that converts filterName to an image source, I think both cases could be handled in the same function, and the duplicate code can be avoided.
| @@ -329,9 +350,12 @@ export const makeDatasetTableHeader = (datasets: DatasetTerm[], selected: number | |||
| cellStyle: makeHeaderStyle(cellWidths.accessType), | |||
| cellDataFn: (dataset: DatasetTerm) => ({ | |||
| data: dataset.accessManagement === 'external' ? | |||
| 'External to DUOS' : | |||
| tooltipIconDisplay(externalAccessIcon, 'external', 'External access request required') : | |||
There was a problem hiding this comment.
This looks like it's duplicating the logic that the filter list uses to map the access management name to the icon path. If the keys are the same in both cases, you could have a single object that maps a name to the icon, type and tooltip text. This code would use all the fields, and the list code would only use the icon path.
| @@ -0,0 +1,11 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
It's a minor thing but these svgs could be optimized. Running this file through https://github.com/svg/svgo (with prettify) I get
<svg width="16" height="16" xmlns="http://www.w3.org/2000/svg">
<path d="M8 0c.147 0 .288.032.419.091l5.884 2.496c.688.292 1.203.97 1.197 1.76-.016 3.128-1.29 8.8-6.675 11.378a1.93 1.93 0 0 1-1.65 0C1.789 13.147.515 7.475.472 4.347c.024-.79.538-1.468 1.225-1.76L7.584.091A.993.993 0 0 1 8 0Zm0 13.9c4.284-2.087 5.472-6.71 5.5-9.481L8 2.087V13.9Z" fill="#354052" fill-rule="nonzero"/>
</svg>
This is the tool that terra-ui uses to optimize its svgs.
| disable={false} | ||
| scrollHide={true} |
There was a problem hiding this comment.
I believe these are the default settings and can be omitted.
Addresses
https://broadworkbench.atlassian.net/browse/DT-789
Summary
Add icons to the Access Type column in the Dataset table in the Data Library
Tooltip appears when you hover over the icon
Screen.Recording.2024-11-15.at.4.25.14.PM.mov
Have you read Terra's Contributing Guide lately? If not, do that first.