8000 Update styling of SourceFilePage content by huangsam · Pull Request #1609 · jacoco/jacoco · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update styling of SourceFilePage content #1609

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
Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Update styling of SourceFilePage content #1609

wants to merge 17 commits into from

Conversation

huangsam
Copy link
@huangsam huangsam commented Apr 7, 2024 •
edited
Loading

Update styling of SourceFilePage content.

Overall goal

By making this change, we can move towards the type of source-code formatting that Golang uses (example) and Rust uses (example) - which is less "box-like" and more minimal. IMO the minimal nature of the new style seems easier to read but I'd be happy to change things up if needed. Overall, this aligns with the spirit of #756 and #1355.

Technical detail

I made changes to the report.css so that we are leveraging CSS variables for different gradients of grey across the HTML report. This introduction improves readability, and also allows for future contributors to define and apply colors consistently.

Developer experience

Took me a little while (but not impossible) to understand this (internal) architecture of the HTML logic:

SourceFilePage -> NodePage -> ReportPage -> ILinkable

SourceFilePage.content
    SourceHighlighter.render
        HTMLElement.pre

Could be useful to put some high-level docs at the ReportPage or NodePage layer to guide folks in the right direction, in case they want to make extensions to any particular Jacoco report.

Items to tackle in separate PRs

Item 1: Some Java files could be changed to some Java files to improve their readability. These Java changes make it easier to understand where to make changes in case folks want to make the jump from Prettify (archived) to Highlight.js.

Item 2: I also noticed that there was a mix of tabs and spaces in various source files (from my changes) due to Intellij's default settings. So we can add an .editorconfig so that all editors which support EditorConfig will respect this styling preference.

Current style (from vanilla run of jacoco right after ./gradlew test):

current-style

New style (done live in Chrome Dev Tools, not from a fresh copy of jacoco - not sure how to do that):

new-one

@huangsam huangsam force-pushed the samhuang/update-source-file-page-styling branch from 4d29482 to 5c70d41 Compare April 7, 2024 05:45
@huangsam
Copy link
Author
huangsam commented Apr 7, 2024 •
edited
Loading

By the way, I have a few side questions:

  1. How is Azure Pipelines serving this project so far? I think y'all are using this at the moment: https://github.com/marketplace/actions/azure-pipelines-action. FYI I'm using CircleCI for my repos so far: example and it's been working okay for my personal needs, but I'm always on the lookout for better solutions

  2. Is there an idea of when Jacoco might drop support of a particular JDK? Here's my thinking: JDK 8, 11, 17, 21 are the latest LTS versions. And JDK 8 is aimed at being EOL'd in 2030. For JDK 7 and below, they have already went on EOL but we still have JDK 5, 6, 7 in the build matrix

@huangsam huangsam changed the title Update styling of SourceFilePage Update styling of SourceFilePage content Apr 7, 2024
barnett-yuxiang
barnett-yuxiang reviewed Apr 7, 2024
View reviewed changes
Copy link
@barnett-yuxiang barnett-yuxiang left a comment

Choose a reason for hiding this comment

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

wowo

@marchof
Copy link
Member
marchof commented Apr 9, 2024

@huangsam Regarding your side questions:

  1. We use Azure pipelines since quite some time, when GitHub Actions was not yet around or had limitations. @Godin Is currently moving builds step-by-step to GitHub actions.
  2. JaCoCo is used in many industries which partly still use ancient Java versions. Unless there is a real blocker for us we keep supporting and testing with Java >= 5.

@huangsam
Copy link
Author
huangsam commented Apr 9, 2024

Thanks for sharing about the details of the CI environment and the commitment to all the JDKs. That helps me understand the background of the project better.

Feel free to review the PR contents at your convenience. I look forward to further collaborations on the HTML component of Jacoco.

@huangsam
Copy link
Author

@marchof any chance that you or another contributor can review the changes here?

@marchof
Copy link
Member
marchof commented Apr 17, 2024

@huangsam Thanks for your contribution and sorry for our silence. This is just a spare time project.

To be honoest, I don't like the new design as the line numbers now have the exact same style as the code. Can we still render the line numbers in a more dim style (like same color as before)?

@marchof
Copy link
Member
marchof commented Apr 17, 2024

@huangsam Because I didn't find the time to check the code myself: Did you check that the fragment references still work? Like this link jumps to the specific method.

https://www.jacoco.org/jacoco/trunk/coverage/org.jacoco.core/org.jacoco.core.data/ExecutionData.java.html#L83

@huangsam
Copy link
Author

@huangsam Thanks for your contribution and sorry for our silence. This is just a spare time project.

To be honoest, I don't like the new design as the line numbers now have the exact same style as the code. Can we still render the line numbers in a more dim style (like same color as before)?

@marchof I addressed the line numbers. Also applied CSS variables to report.css so it's clear what gradient of grey we're applying for different parts of the HTML report.

@huangsam
Copy link
Author
huangsam commented Apr 17, 2024 •
edited
Loading

@huangsam Because I didn't find the time to check the code myself: Did you check that the fragment references still work? Like this link jumps to the specific method.

https://www.jacoco.org/jacoco/trunk/coverage/org.jacoco.core/org.jacoco.core.data/ExecutionData.java.html#L83

@marchof yes I checked that the fragment references are still respected. My CSS changes are only adjustments to existing CSS classes. I don't remove existing line-by-line CSS such as L0, L1, etc.

fragment-jump-works

@marchof
Copy link
Member
marchof commented Apr 22, 2024

Hi @huangsam Looks like there is many aspects now in this PR (editor settings, Java code refactoring, changing the style, introdusing css variables). Please try to do one thing at a time in a PR. So the PR has clear scope and we can focus on the solution of that specific problem.

@huangsam
Copy link
Author

Fair point @marchof. If anything, the essential changes for this PR are from the CSS file. Everything else is optional and can be added in later PRs

@huangsam
Copy link
Author

@marchof there's only one file with diffs now

@huangsam
Copy link
Author

@marchof hello. Is there anything else that concerns you regarding the changes in this PR?

@huangsam huangsam force-pushed the samhuang/update-source-file-page-styling branch from 62ed575 to a3bcce3 Compare June 22, 2024 05:54
@huangsam
Copy link
Author

@marchof @Godin hello. I wanted to check back in to see if there were any more outstanding concerns for the changes in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barnett-yuxiang barnett-yuxiang barnett-yuxiang left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@huangsam @marchof @barnett-yuxiang
0