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.

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

feat: offer multiple sorting options for attributions #2481

Merged
merged 1 commit into from Jan 10, 2024

Conversation

antonbauhofer
Copy link
Member

Summary of changes

add toggle state for critical signals progress bar to store
extend sorting algorithm to consider criticality
extend test of sorting
Context and reason for change

There is highlighting for critical signals but before this change it was still hard to find critical signals in long lists of signals coming from e.g. ScanCode.

How can the changes be tested

Open an input file that contains critical signals and look at the sorting.

@antonbauhofer antonbauhofer force-pushed the feat-add-sorting-options branch 2 times, most recently from ad706ad to 049fe76 Compare January 9, 2024 16:33
byCriticality: 'By Criticality',
},
tooltips: {
search: 'Search signals by name, license name, copyright text and version',
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to group semantically than by technical function. so this would be part of "attributionColumn".

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it as suggested. However, I don't quite agree with you here. The name "attributionColumn" indicates grouping the texts by components. Hence, we will either eventually end up defining the same text multiple times or using texts from other components. The name "tooltips", on the other hand, can be understood as the user-facing behavior in the frontend. So these texts could, for example be used for the tooltips in the auditView and in the attributionView.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, good point. one needs to see if the text is reused or not. it seems to me this one is not. what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case it probably doesn't matter, since we are using different texts anyway. But in general I don't think that it is a good idea to start grouping the texts by components.

@@ -44,6 +44,8 @@ export const text = {
source: 'Source',
useAutocompleteSuggestion:
'Adopt all coordinates and legal information from suggestion',
sortTooltip: 'Sort',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to say "Sorting" here because then we match the noun "Filter" instead of using the verb "sort".

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I read "filter" as a verb too. In this case it matches, that's why I chose "sort".

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, but it currently says "filters", not "filter"

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 removed the "s"

@antonbauhofer antonbauhofer force-pushed the feat-add-sorting-options branch 2 times, most recently from 6cfe0db to 2fb5ac4 Compare January 10, 2024 09:27
… selected

- add toggle state for critical signals progress bar to store
- extend sorting algorithm to consider criticality
- only sort by criticality if criticality progress bar is selected
- extend test of sorting
- fix hydration condition of signals web-worker

Signed-off-by: Anton Bauhofer <anton.bauhofer@tngtech.com>
@antonbauhofer antonbauhofer merged commit 60f3b51 into main Jan 10, 2024
5 checks passed
@antonbauhofer antonbauhofer deleted the feat-add-sorting-options branch January 10, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve visibility of critical signals in long lists of signals
2 participants