Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Conversation

@uhafner
Copy link
Member

@uhafner uhafner commented May 20, 2022

Main table

Bildschirmfoto 2022-05-23 um 08 49 38

Coverage table

Bildschirmfoto 2022-05-23 um 08 50 26

@uhafner uhafner added the enhancement Enhancement of existing functionality label May 20, 2022
@uhafner uhafner requested a review from fo-code May 20, 2022 15:27
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #394 (bf50754) into master (bfaf7e5) will decrease coverage by 1.29%.
The diff coverage is 14.34%.

@@             Coverage Diff              @@
##             master     #394      +/-   ##
============================================
- Coverage     72.74%   71.45%   -1.30%     
- Complexity      976      980       +4     
============================================
  Files            82       86       +4     
  Lines          3632     3713      +81     
  Branches        432      436       +4     
============================================
+ Hits           2642     2653      +11     
- Misses          847      914      +67     
- Partials        143      146       +3     
Impacted Files Coverage Δ
...enkins/plugins/coverage/model/SourceViewModel.java 0.00% <0.00%> (ø)
...age/model/visualization/code/SourceCodeFacade.java 85.06% <ø> (ø)
...ins/plugins/coverage/model/CoverageTableModel.java 9.30% <9.30%> (ø)
...ugins/coverage/model/ChangeCoverageTableModel.java 9.37% <9.37%> (ø)
...s/coverage/model/IndirectCoverageChangesTable.java 10.34% <10.34%> (ø)
...kins/plugins/coverage/model/CoverageViewModel.java 60.19% <32.43%> (+29.49%) ⬆️
...kins/plugins/coverage/model/CoverageXmlStream.java 79.74% <71.42%> (-2.45%) ⬇️
...nkins/plugins/coverage/targets/CoverageResult.java 33.00% <0.00%> (-0.66%) ⬇️

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 bfaf7e5...bf50754. Read the comment docs.

Copy link
Collaborator

@fo-code fo-code left a comment

Choose a reason for hiding this comment

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

The new design looks a lot better!
Just one little thing that i noticed...

<j:choose>
<j:when test="${coverageValue.isPresent()}">
<j:set var="displayColors" value="${it.getDisplayColors(job, coverageValue)}"/>
<j:set var="lineColor" value="${displayColors.getLineColorAsHex()}"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The intention behind this line is that it is guaranteed that the color of the text is readable on the background color.
With the currently used color palette, this shouldn't be a problem.
I guess this has been removed to make all URLs in the dashboard the same color?
When this line is removed, we have to take care that there are no background colors in use which causes problems with the readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

When we set a color then we should use the predefined color variables of Jenkins. Without the color, the links are using the Jenkins default, which should look good in all themes:

Bildschirmfoto 2022-05-22 um 22 11 24

${coverageText}
</j:when>
<j:otherwise>
<a style="color:${lineColor}" href="${rootURL}/${job.url}lastSuccessfulBuild/${url}">${coverageText}</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

style="color:${lineColor}" can be removed if there should always be the default URL color in use.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. I just deleted the variable and forgot to remove the reference.

String cell = div().withClasses(COVERAGE_COLUMN_OUTER).with(
div().withClasses(COVERAGE_COLUMN_INNER)
.withStyle(String.format(
"color:%s; background-image: linear-gradient(90deg, %s %f%%, transparent %f%%);",
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 think here we have the same problem. The whole UI looks somewhat funny in dark mode (I need to update the datatables CSS as well)

Bildschirmfoto 2022-05-22 um 22 36 02

Copy link
Collaborator

@fo-code fo-code left a comment

Choose a reason for hiding this comment

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

Apart from the problem with the dark mode view, the new design of the columns looks very good to me!

@uhafner
Copy link
Member Author

uhafner commented Jun 8, 2022

Yes, the dark mode needs to be fixed in an additional PR.

PS: I did not merge the changes yet since the layout still needs to be improved for small screens. Currently the source code and the tables are only visible by 50% when used on a laptop. I started refactoring the data tables plugin so that superfluous columns will be hidden on small screens automatically. The works quite well already (not yet pushed). Hopefully I find some time next week to improve the source code view for small views as well. I think for those screens it might make sense to restore the old behavior of having a separate view for the source code and one for the table. Or do you have a better idea, @fo-code?

@fo-code
Copy link
Collaborator

fo-code commented Jun 9, 2022

Yes you are right.
This is a problem I already tried to fix in some way, using the width option for columns in CoverageViewModel.
That didn't work for the reason I mentioned in the TODOs there, but it can enhance the view for small screens.
I already tried that manuelly.
Anyway, I guess your approach sounds good since there might be additional columns in the future regarding complexity etc..

One option is that we could add an option to hide the integrated source code view, as already done for the coverage charts within the overview.
When the integrated source code is activated, everything works as it is right now.
Otherwise (when the integrated source code view is deactivated using the toggle button) the row selection calls the former method which opens a new tab - we can check this Javascript - and the table takes the whole screen.

Another option would be to always show the source code view next to the table, but put the focus on the table that it has always its required size - the source code is then only visible on small screens using horizontal scrolling.
Then, add links to the file names to provide the option to open the source code in a new tab, as it has be done bedore.

The most easy option is, that we change only the UI that the source code is shown below the table if the screen is to small to show both cards next to each other in the required size.
I my opinion, that has the same effect as opening a new tab, but it requires no changes of the logic, additional tests etc..

I think these are approaches which covers every use case and can be integrated quite easily, since the code for opening new tabs already exist.
I would prefer the last mentioned option, since it is already done like this for other parts of Jenkins - at least that's what I saw.
What do you think about that @uhafner?

uhafner and others added 13 commits June 9, 2022 16:53
Bumps [analysis-pom](https://github.com/jenkinsci/analysis-pom-plugin) from 5.26.0 to 5.28.0.
- [Release notes](https://github.com/jenkinsci/analysis-pom-plugin/releases)
- [Changelog](https://github.com/jenkinsci/analysis-pom-plugin/blob/master/CHANGELOG.md)
- [Commits](jenkinsci/analysis-pom-plugin@v5.26.0...v5.28.0)

---
updated-dependencies:
- dependency-name: org.jvnet.hudson.plugins:analysis-pom
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [codingstyle-pom](https://github.com/uhafner/codingstyle-pom) from 2.27.0 to 2.28.0.
- [Release notes](https://github.com/uhafner/codingstyle-pom/releases)
- [Changelog](https://github.com/uhafner/codingstyle-pom/blob/main/CHANGELOG.md)
- [Commits](uhafner/codingstyle-pom@v2.27.0...v2.28.0)

---
updated-dependencies:
- dependency-name: edu.hm.hafner:codingstyle-pom
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Make the UI views responsive so that the source code is shown on the same page when possible.
@uhafner uhafner merged commit d3c6d64 into master Jun 22, 2022
@uhafner uhafner deleted the column-design-improvement branch June 22, 2022 22:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Enhancement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants