Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #86 by removing MarkdownCodeBlock’s dedicated user-agent stylesheet (which prevented downstream CSS overrides) and consolidating code block styling into the main mdfx.css, along with some CSS cleanup/simplification.
Changes:
- Removed
MarkdownCodeBlock’sgetUserAgentStylesheet()and deletedmarkdown-code-block.css. - Refactored
mdfx.cssvariables/styles (now scoped under.markdown-view) and added/updated block quote + code block rules. - Updated the example to apply padding and add the
markdown-viewstyle class.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
jpro-mdfx/src/main/resources/one/jpro/platform/mdfx/mdfx.css |
Consolidates styling (incl. code blocks) and refactors default looked-up variables. |
jpro-mdfx/src/main/resources/one/jpro/platform/mdfx/markdown-code-block.css |
Removed to avoid a separate UA stylesheet for code blocks. |
jpro-mdfx/src/main/java/one/jpro/platform/mdfx/MarkdownCodeBlock.java |
Removes the control-specific UA stylesheet hook. |
jpro-mdfx/example/src/main/java/one/jpro/platform/mdfx/example/MarkdownViewSample.java |
Applies padding/style class to match the new CSS scoping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| .markdown-view { | ||
| -mdfx-font-color: -fx-text-base-color; | ||
| -mdfx-link-color: -fx-accent; | ||
| -mdfx-border-color-1: -fx-box-border; | ||
|
|
||
| -mdfx-bg-color-1: #ccc; | ||
| -mdfx-bg-color-2: #ddd; | ||
| -mdfx-bg-color-3: #eee; | ||
| -mdfx-bg-color-1: -fx-control-inner-background; | ||
| -mdfx-bg-color-2: -fx-control-inner-background-alt; | ||
| -mdfx-bg-color-3: -fx-color; | ||
|
|
||
| -mdfx-bq-color-border: #4488cc; | ||
| -mdfx-bq-color-background: #0000ff0c; | ||
| -mdfx-bq-color-border: derive(-fx-accent, -50%); | ||
| -mdfx-bq-color-background: -fx-accent; | ||
| -mdfx-bq-text-fill: -fx-selection-bar-text; | ||
|
|
||
| -mdfx-code-block-border: -fx-box-border; | ||
| -mdfx-code-block-background: -fx-control-inner-background; | ||
| } |
There was a problem hiding this comment.
Changing the default variable definitions from the global * selector to .markdown-view will break styling unless MarkdownView instances always have the markdown-view style class. MarkdownView currently does not add this class by default (it’s a plain VBox), so looked-up colors like -mdfx-font-color may become undefined for existing consumers. Consider restoring a global fallback selector, or (preferably) ensure MarkdownView adds markdown-view to its style class list in code so the CSS always applies without requiring app code changes.
| -mdfx-bg-color-3: #eee; | ||
| -mdfx-bg-color-1: -fx-control-inner-background; | ||
| -mdfx-bg-color-2: -fx-control-inner-background-alt; | ||
| -mdfx-bg-color-3: -fx-color; |
There was a problem hiding this comment.
-mdfx-bg-color-3 is set to -fx-color, but this looked-up color is not defined anywhere in this project’s CSS and may not be available for a VBox-based control under all JavaFX themes. If it resolves to an undefined value, .markdown-table-cell-top background styling can be ignored. Consider deriving it from a reliably-defined value like -fx-control-inner-background (e.g., derive(...)) or using -fx-control-inner-background directly.
| -mdfx-bg-color-3: -fx-color; | |
| -mdfx-bg-color-3: -fx-control-inner-background; |
| -fx-background: -mdfx-bq-color-background; | ||
| -fx-background-color: -fx-background; |
There was a problem hiding this comment.
Using -fx-background as an intermediate looked-up value is confusing because the -fx- prefix is generally reserved for JavaFX-defined properties, and JavaFX has no standard -fx-background property (only -fx-background-color, -fx-background-insets, etc.). Since you already have -mdfx-bq-color-background, it’s clearer and less error-prone to set -fx-background-color directly to -mdfx-bq-color-background and drop the extra looked-up value.
| -fx-background: -mdfx-bq-color-background; | |
| -fx-background-color: -fx-background; | |
| -fx-background-color: -mdfx-bq-color-background; |
| import one.jpro.platform.mdfx.MarkdownView; | ||
| import one.jpro.platform.mdfx.extensions.YoutubeExtension; | ||
| import org.apache.commons.io.IOUtils; | ||
| import org.scenicview.ScenicView; |
There was a problem hiding this comment.
org.scenicview.ScenicView is imported but not used anywhere in this file, which will fail builds that enforce no-unused-imports (e.g., via Checkstyle/Spotless) and adds unnecessary dependency coupling in the example. Please remove the unused import or add the intended ScenicView usage.
| import org.scenicview.ScenicView; |
Fixes issue #86 and some more cleanup / simplification.