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

[MSKINS-245] Add code highlighting class to every <pre><code> combination #58

Merged

Conversation

kwin
Copy link
Member

@kwin kwin commented Mar 11, 2024

Don't rely on custom div classes surrounding those as those are not always set.

@kwin kwin requested a review from michael-o March 11, 2024 08:07
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

This is broken as it does not reflect the skin structure AND the CSS anymore.

@kwin
Copy link
Member Author

kwin commented Mar 11, 2024

This is therefore only a draft to outline how this should work IMHO on the Skin side. Would you mind commenting on the general approach to only rely on <pre><code> for block codes?

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

You missed to change:

section > div.verbatim {
margin-right: 7px;
margin-left: 7px;
}

@kwin kwin force-pushed the bugfix/add-code-highlightling-for-all-pre-code-combinations branch from e1e9180 to e9c224a Compare March 15, 2024 16:04
@kwin kwin changed the title MSKINS-1002 Add code highlighting class to every <pre><code> combination [MSKINS-245] Add code highlighting class to every <pre><code> combination Mar 15, 2024
@kwin kwin marked this pull request as ready for review March 15, 2024 16:04
@kwin
Copy link
Member Author

kwin commented Mar 15, 2024

This also fixes the missing margin for <div class="source"> (broken in both Fluido 1.x and 2.x), which e.g. can be seen in https://jackrabbit.apache.org/filevault-package-maven-plugin/index.html

Screenshot 2024-03-15 at 17 08 35

@kwin kwin requested a review from michael-o March 15, 2024 16:12
@kwin kwin force-pushed the bugfix/add-code-highlightling-for-all-pre-code-combinations branch from e9c224a to 0ebfdfe Compare March 15, 2024 16:38
@michael-o michael-o marked this pull request as draft March 16, 2024 08:19
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Althrough this looks good and right, it needs to be remain a draft until the fixed Doxia (Sitetools) version is released this can be bumped:

<doxia-sitetools>2.0.0-M12</doxia-sitetools>

@kwin kwin force-pushed the bugfix/add-code-highlightling-for-all-pre-code-combinations branch from 0ebfdfe to 2b56992 Compare April 29, 2024 16:54
@kwin kwin marked this pull request as ready for review April 29, 2024 16:55
@kwin kwin marked this pull request as draft April 29, 2024 17:01
@kwin
Copy link
Member Author

kwin commented Apr 29, 2024

Still requires a m-site-p version depending on m-doxia-sitetools M18+...

@michael-o
Copy link
Member

This can now reduced to:

diff --git a/src/main/resources/META-INF/maven/site.vm b/src/main/resources/META-INF/maven/site.vm
index 5e0604f..26bb876 100644
--- a/src/main/resources/META-INF/maven/site.vm
+++ b/src/main/resources/META-INF/maven/site.vm
@@ -242 +242 @@
-#*      *#$bodyContent.replaceAll( '<div class="verbatim source">(\r?\n)?<pre>', '<div class="verbatim source"><pre class="' + $sourceStyle + '">' ).replaceAll( 'class="bodyTable"', 'class="table table-striped"' ).replaceAll( 'class="bodyTable bodyTableBorder"', 'class="table table-bordered table-striped"' )
+#*      *#$bodyContent.replace( '<pre><code>', '<pre class="' + $sourceStyle + '"><code>' ).replace( 'class="bodyTable"', 'class="table table-striped"' ).replace( 'class="bodyTable bodyTableBorder"', 'class="table table-bordered table-striped"' )
diff --git a/src/main/resources/css/maven-base.css b/src/main/resources/css/maven-base.css
index 829ef71..5747cd6 100644
--- a/src/main/resources/css/maven-base.css
+++ b/src/main/resources/css/maven-base.css
@@ -33 +33 @@ section > table.table,
-section > div.verbatim {
+section > pre {

@michael-o
Copy link
Member

@kwin Should I rebase and merge this? I'd like to start the release.

@kwin
Copy link
Member Author

kwin commented May 19, 2024

Should I rebase and merge this? I'd like to start the release

Yes, please.

…tion

Don't rely on custom div classes surrounding those.

This closes #58
@asfgit asfgit force-pushed the bugfix/add-code-highlightling-for-all-pre-code-combinations branch from 2b56992 to ab1e6a3 Compare May 20, 2024 08:23
@michael-o michael-o self-requested a review May 20, 2024 08:24
@michael-o michael-o marked this pull request as ready for review May 20, 2024 08:24
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Will merge as soon as CI completes.

@asfgit asfgit closed this in ab1e6a3 May 20, 2024
@asfgit asfgit merged commit ab1e6a3 into master May 20, 2024
30 checks passed
@michael-o michael-o deleted the bugfix/add-code-highlightling-for-all-pre-code-combinations branch May 20, 2024 08:34
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.

3 participants