Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add search icons to CriticalLicensesTable in ProjectStatisticsPopup #1997

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Add search icons to CriticalLicensesTable in ProjectStatisticsPopup #1997

merged 1 commit into from
Sep 20, 2023

Conversation

lennartclaas
Copy link
Member

Summary of changes

This commit adds icons for searching licenses to the CriticalLicensesTable in ProjectStatisticsPopup. The logic is implemented in an upcoming PR.

Context and reason for change

We want the user to be able to search for resources with specific licenses via the ProjectStatisticsPopup. The new search icons allow for searching licenses in the ResourceBrowser according to the functionality of the LocatorPopup.

How can the changes be tested

Automatic testing:
Run ProjectStatisticsPopup.test.tsx.
Manual testing:
Load test file, open ProjectStatisticsPopup, and hover over the icons in the third column of the CriticalLicensesTable.

Fix: #1984

benedikt-richter
benedikt-richter reviewed Sep 18, 2023
View reviewed changes
src/Frontend/Components/Icons/Icons.tsx Outdated
@@ -73,6 +74,10 @@ interface LabelDetailIconProps extends IconProps {
disabled?: boolean;
}

interface SearchLicenseIconProps extends IconProps {
Copy link
Member

Choose a reason for hiding this comment

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

We typically call it "locate". I.e. LocateLicense...

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed everything according to LocatorPopup. In particular, it is searching for attributions (see button label). I guess to keep everything as generic as possible (one could think about searching for attributions via licenses and other properties).

benedikt-richter
benedikt-richter reviewed Sep 18, 2023
View reviewed changes
src/Frontend/Components/CriticalLicensesTable/CriticalLicensesTable.tsx Outdated
};
const buttonIsEnabled = Boolean(totalNumberOfAttributions);
const tooltipTitle = buttonIsEnabled
? `search for "${licenseName}" in resource browser`
Copy link
Member

Choose a reason for hiding this comment

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

It's not a "search" but a "locate" in our terms

Copy link
Contributor

@vasily-pozdnyakov vasily-pozdnyakov Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

and I am not sure that every user knows what resource browser is. I propose something like: "locate resources with 'some license' "

@benedikt-richter
Copy link
Member

Maybe it would be more consistent to use the "locate" icon instead of the search icon

vasily-pozdnyakov
vasily-pozdnyakov requested changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@vasily-pozdnyakov vasily-pozdnyakov left a comment

Choose a reason for hiding this comment

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

Honestly, I do not like the appearance (not sure that new column is required, also Total looks not good in this case)
image

src/Frontend/Components/CriticalLicensesTable/CriticalLicensesTable.tsx Outdated
};
const buttonIsEnabled = Boolean(totalNumberOfAttributions);
const tooltipTitle = buttonIsEnabled
? `search for "${licenseName}" in resource browser`
Copy link
Contributor

@vasily-pozdnyakov vasily-pozdnyakov Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

and I am not sure that every user knows what resource browser is. I propose something like: "locate resources with 'some license' "

@vasily-pozdnyakov
Copy link
Contributor

vasily-pozdnyakov commented Sep 18, 2023 •
edited
Loading

Try something like
image
or
image

@lennartclaas
Copy link
Member Author

I decided to place the icons after the license name because I want to preserve text alignment on the left side of the first column.

Screenshot 2023-09-18 181741

@benedikt-richter benedikt-richter self-assigned this Sep 19, 2023
benedikt-richter
benedikt-richter reviewed Sep 19, 2023
View reviewed changes
src/Frontend/Components/ProjectLicensesTable/ProjectLicensesTable.tsx Outdated
@@ -19,6 +19,12 @@ import { LicenseNamesWithCriticality } from '../../types/types';

const PLACEHOLDER_ATTRIBUTION_COUNT = '-';

const classes = {
rowNameWithIconButton: {
marginRight: '8px',
Copy link
Member

Choose a reason for hiding this comment

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

I'd attach the margin to the button instead. The butten itself should take care of it.

benedikt-richter
benedikt-richter approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@benedikt-richter benedikt-richter left a comment

Choose a reason for hiding this comment

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

just minor comment

vasily-pozdnyakov
vasily-pozdnyakov approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@vasily-pozdnyakov vasily-pozdnyakov left a comment

Choose a reason for hiding this comment

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

Just a minor thing about naming:
do we locate attributions or resources?

src/Frontend/Components/CriticalLicensesTable/CriticalLicensesTable.tsx
};
return (
<IconButton
tooltipTitle={`locate attributions with "${licenseName}"`}
Copy link
Contributor

Choose a reason for hiding this comment

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

why attributions? should it be resources?

Copy link
Member

Choose a reason for hiding this comment

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

I, personally, would argue that it is signals that we are locating.

Copy link
Member Author

Choose a reason for hiding this comment

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

To my understanding, we locate attributions (license is a property of an attribution). As attributions are attached to resources, we locate resources at the same time. It's correct that the visual hints (arrows) point to resources, but I see that as some kind of incompleteness of the location approach we employ. Ideally, we would mark the located attributions, not only the resources. But that would be overkill as the user can easily identify the attribution of interest given the correct resource. Regarding the tooltip, a correct title would be something like 'locate resources that have attributions with attached'. But I don't like that.

@lennartclaas
Add search icons to CriticalLicensesTable in ProjectStatisticsPopup
64e452b 
The new search icons allow for searching licenses in the `ResourceBrowser`
according to the functionality of the `LocatorPopup`. This commit only
adds the icons. The logic is implemented in an upcoming PR.

Fix: opossum-tool#1984

Signed-off-by: Lennart Holstein <lennart.holstein@tngtech.com>
@lennartclaas lennartclaas merged commit c6d125e into opossum-tool:main Sep 20, 2023
@lennartclaas lennartclaas deleted the search-license-icon branch September 27, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benedikt-richter benedikt-richter benedikt-richter approved these changes

@vasily-pozdnyakov vasily-pozdnyakov vasily-pozdnyakov approved these changes

Assignees

@benedikt-richter benedikt-richter

@vasily-pozdnyakov vasily-pozdnyakov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[1952] Add icons to "critical licenses" table
3 participants
@lennartclaas @benedikt-richter @vasily-pozdnyakov