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

Axis and renderer CSS Styling #598

Merged
merged 19 commits into from
Aug 10, 2023
Merged

Axis and renderer CSS Styling #598

merged 19 commits into from
Aug 10, 2023

Conversation

ennerf
Copy link
Collaborator

@ennerf ennerf commented Aug 10, 2023

This PR includes some styling changes for the components that are comparatively easy to deal with. Datasets will require more work and will be in a separate PR.

  • added SCSS integration
  • made axes fully styleable (including the side)
  • fixed CSS styling for the grid renderer
  • added CSS styling for the error renderer
  • removed duplicate properties
  • removed many convenience methods for styling, e.g., setLabelFill(color)->getLabel.setFill(color)`. This may result in a bit more effort to port, but it cleans up the API and should help encourage the use of CSS.
  • added extra pseudo classes for sides (e.g. vertical, horizontal, center)

Bugfixes

  • Fixes Fix CSS styling for GridRenderer #560 grid styling
  • fixed some tick-label artifacts caused by rendering labels outside the parent pane (e.g. the bottom-left x-axis tick if the y axis is on the right)

Demos with Sass and live CSS reloading

Notes

  • The CssPropertyFactory in some Axes (e.g. NumericAxis) gets initialized with the static properties from the parent class before they are valid. This is a bug in current main.
  • The legend initializes and renders the symbols before the CSS info is available. The symbols should be updated in the draw phase.

@pr-explainer-bot
Copy link

Pull Request Review Markdown

Hey there! 👋 Here's a summary of the changes, suggestions, bugs, improvements, and rating for the pull request. Let's dive in!

Changes

  1. Added properties sass.version, scss.inputDir, and css.outputDir in the properties section of the pom.xml file.
  2. Added a build section in the pom.xml file with a sass-cli-maven-plugin plugin configuration.
  3. Added a run goal in the executions section of the sass-cli-maven-plugin plugin configuration.
  4. Removed the isLegendVisible() method from the Chart class.
  5. Deprecated the legendVisibleProperty() method in the Chart class.
  6. Added a getLegend().getNode().visibleProperty() call in the legendVisibleProperty() method of the Chart class.
  7. Removed the setLegendSide() method from the Chart class.
  8. Removed the legendSideProperty() method from the Chart class.
  9. Removed the setTitleSide() method from the Chart class.
  10. Removed the setTitlePaint() method from the Chart class.
  11. Deprecated the legendVisibleProperty() method in the Chart class.
  12. Added a getLegend().getNode().setVisible(value) call in the setLegendVisible() method of the Chart class.
  13. Removed the titleProperty() method from the Chart class.
  14. Removed the method horizontalGridLinesVisibleProperty() and the corresponding getter isHorizontalGridLinesVisible() from class XYChart.
  15. Removed the method verticalGridLinesVisibleProperty() and the corresponding getter isVerticalGridLinesVisible() from class XYChart.

Suggestions

  1. Line 5: Added imports for classes AbstractRenderer, StyleGroup, TitleLabel.
  2. Line 7: Removed import for class Label.
  3. Line 29: Added styles to plotBackground, plotArea, plotForeGround.
  4. Line 32: Added styles to measurementPane, titleLegendPane, axesAndCanvasPane.
  5. Line 102: Removed legendVisible property and related code.
  6. Line 123: Removed titleLabel and replaced it with TitleLabel.
  7. In XYChart class, lines 88-91, the method setHorizontalGridLinesVisible() can be removed as it is no longer used.
  8. In XYChart class, lines 221-224, the method setVerticalGridLinesVisible() can be removed as it is no longer used.

Bugs

  1. In GridRenderer class, lines 47-49, the line dashes are set to DEFAULT_GRID_DASH_PATTERM but it seems that the code to set the line dashes is commented out. This could be a potential bug.
  2. In ReducingLineRenderer class, line 32, the class extends AbstractRenderer but the comment mentions AbstractDataSetManagement. This could be a potential bug.

Improvements

  1. In AbstractPointReducingRenderer class, lines 25-29, the class extends AbstractRenderer and overrides the css() method. This code can be refactored for better readability as follows:
@Override
protected CssPropertyFactory

Rating

Overall, the code changes look good, with some minor suggestions and a couple of potential bugs. I would rate the code a 7 out of 10 based on readability, performance, and security.

That's it for the pull request review! Let me know if you need any further assistance. 😄

wirew0rm
wirew0rm previously approved these changes Aug 10, 2023
Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

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

Very nice work, looks good and can be merged once the tests are fixed.

@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to coverage August 10, 2023 16:09 — with GitHub Actions Inactive
@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 11 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 33 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@wirew0rm wirew0rm temporarily deployed to deploy August 10, 2023 16:13 — with GitHub Actions Inactive
@wirew0rm wirew0rm temporarily deployed to deploy August 10, 2023 16:13 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 74.93% and project coverage change: -0.56% ⚠️

Comparison is base (110f856) 50.27% compared to head (af73ef2) 49.71%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #598      +/-   ##
============================================
- Coverage     50.27%   49.71%   -0.56%     
+ Complexity     6268     6246      -22     
============================================
  Files           375      379       +4     
  Lines         37531    37625      +94     
  Branches       6143     6165      +22     
============================================
- Hits          18867    18707     -160     
- Misses        17414    17687     +273     
+ Partials       1250     1231      -19     
Files Changed Coverage Δ
...c/main/java/io/fair_acc/chartfx/legend/Legend.java 100.00% <ø> (ø)
...r/spi/AbstractContourDataSetRendererParameter.java 100.00% <ø> (ø)
...c/chartfx/renderer/spi/LabelledMarkerRenderer.java 83.10% <ø> (-0.56%) ⬇️
...acc/chartfx/renderer/spi/ReducingLineRenderer.java 0.00% <ø> (ø)
...c/main/java/io/fair_acc/chartfx/utils/FXUtils.java 88.23% <0.00%> (-3.61%) ⬇️
.../java/io/fair_acc/chartfx/utils/RotatedBounds.java 17.85% <17.85%> (ø)
.../java/io/fair_acc/chartfx/utils/RotatedRegion.java 33.33% <33.33%> (ø)
...java/io/fair_acc/chartfx/ui/layout/TitleLabel.java 65.00% <65.00%> (ø)
...ain/java/io/fair_acc/chartfx/ui/css/StyleUtil.java 68.75% <66.66%> (-29.44%) ⬇️
.../io/fair_acc/chartfx/legend/spi/DefaultLegend.java 84.46% <71.87%> (-6.11%) ⬇️
... and 17 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wirew0rm wirew0rm merged commit a0cbf84 into main Aug 10, 2023
10 of 12 checks passed
@wirew0rm wirew0rm deleted the ennerf/styling branch August 10, 2023 16:52
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.

Fix CSS styling for GridRenderer
2 participants