Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

GUI miscellaneous #2 #995

Merged
merged 12 commits into from
Oct 4, 2018
Merged

GUI miscellaneous #2 #995

merged 12 commits into from
Oct 4, 2018

Conversation

swaterkamp
Copy link
Member

@swaterkamp swaterkamp commented Oct 4, 2018

  • Fix ScanConfigDialog tables
  • Fix translating FilterTypes
  • Fix CVEs table
  • Change titles in Result details
  • ... some clean-up

Remove "Vulnerability" from titles for "Detection Method", "Insight", 
and "Detection Result". Also don't distinguish between "Detection 
Method" and "Log Method" anymore.
@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #995 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #995      +/-   ##
=========================================
- Coverage    9.11%   9.11%   -0.01%     
=========================================
  Files         821     821              
  Lines       26742   26745       +3     
  Branches     5719    5745      +26     
=========================================
  Hits         2437    2437              
- Misses      21943   21945       +2     
- Partials     2362    2363       +1
Impacted Files Coverage Δ
gsa/src/gmp/models/cve.js 4.16% <ø> (ø) ⬆️
gsa/src/web/pages/cves/row.js 0% <ø> (ø) ⬆️
gsa/src/web/components/table/data.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/results/details.js 0% <0%> (ø) ⬆️
gsa/src/gmp/models/tag.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/filters/component.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/scanconfigs/editdialog.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/results/detailspage.js 0% <0%> (ø) ⬆️
gsa/src/web/components/link/blanklink.js 0% <0%> (ø) ⬆️
gsa/src/web/components/form/download.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e906efe...86b17a1. Read the comment docs.

@@ -327,31 +328,33 @@ class NvtFamily extends React.Component {
{name}
</TableData>
<TableData>
<Layout align="end">
<Layout align="start">
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the align be set on the TableData here? For me the extra Layout seems to be unnecessary.

@@ -84,10 +85,10 @@ class NvtPreferenceDisplay extends React.Component {
<TableData style={{overflowWrap: 'break-word'}}>
{preference.name}
</TableData>
<TableData>
<TableData style={{overflowWrap: 'break-word'}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract this into an own component now we even use this twice?

{/* add empty cells to spread row to end of table */}
<TableData/>
<TableData/>
<TableData/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems strange to me. Shouldn't we remove the columns at all if we don't have content for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have content for the columns, just not in the very last row.

</DetailsLink>
))}
{userTags.map(tag => {
const valueString = tag.value === '' ? '' : '=' + tag.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use isEmpty(tag.value) from gmp/utils/string instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would even better to parse the tag value as undefined if it is an empty string...

@@ -42,6 +42,9 @@ class Tag extends Model {
else {
ret.resourceCount = 0;
}
if (elem.value === '') {
ret.value = undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should be

ret.value = isEmpty(elem.value) ? undefined : elem.value;

{preference.name}
</TableData>
<TableData style={{overflowWrap: 'break-word'}}>
<TableData overflowWrap="break-word">
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry you did misunderstood my comment. Should have been

const StyledTableData = styled(TableData)`
  overflow-wrap: break-word;
`;

...

<StyledTableData>
  {preference.nvt.name}
</StyledTableData>
<StyledTableData>
  {preference.name}
</StyledTableData>
<TableData>
...

Hint: If this doesn't work currently TableData has to forwards the className prop.

Also use this change in Results detailspage
@bjoernricks bjoernricks self-requested a review October 4, 2018 14:32
@swaterkamp swaterkamp merged commit e8a7bf3 into greenbone:master Oct 4, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants