Fix header image size and red line alignment by SheikhZaeem · Pull Request #16256 · checkstyle/checkstyle

SheikhZaeem · 2025-01-28T18:09:07Z

This pull request solves the issue #16243, where the header image was larger than intended and also the red line was not touching the top of the page. The changes include:

Fixing the size of header image.

Adjusting the CSS to ensure the red line aligns properly with the top of the page.

Thank you.

Issue Link:
Fixes: #16243

Zopsss · 2025-01-29T05:10:14Z

github, generate site

github-actions · 2025-01-29T05:13:02Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ada73e0_20250129051210/index.html

Zopsss · 2025-01-29T05:20:38Z

+#bannerRight img {
+  transform: translateY(-12px);
+}
+
 #banner {
  border-bottom: 1px solid #fff;
  background: url("../images/header-background.png") repeat-x scroll 0 0 transparent;
+  transform: translateY(-6px);


you're transforming the banner to upwards, it is causing banner to have more empty space below it.

Current banner:

Banner after your changes:

We want the red line to stick to the top, the problem is that it is wrapped inside H1 tag which have margin-top & bottom.

Removing margin-top will fix the problem

Zopsss · 2025-01-29T05:22:17Z

@romani why do we have this red line? and why it is a link? If the red line is only for aesthetics then it doesn't have to be a link

romani · 2025-01-29T06:10:34Z

why do we have this red line?

This is us how kids use to study to write.
https://www.istockphoto.com/vector/vector-opened-school-cursive-worksheet-copybook-gm872316754-243658512

why it is a link?

I surprised, it supposed to be fancy decor. Link can be removed.

SheikhZaeem · 2025-01-29T08:01:08Z

@Zopsss Thank you very much for the feedback.

Zopsss · 2025-01-29T08:05:31Z

I surprised, it supposed to be fancy decor. Link can be removed.

@SheikhZaeem it would be very helpful if you can remove the <a> from the red line. It will require some JS. If you cannot do it, that's okay but if you want to do it, then you can ask me for help.

SheikhZaeem · 2025-01-29T08:24:27Z

@Zopsss Yes I want to do it. Thank you

SheikhZaeem · 2025-01-29T10:16:10Z

@SheikhZaeem it would be very helpful if you can remove the <a> from the red line. It will require some JS. If you cannot do it, that's okay but if you want to do it, then you can ask me for help.

@Zopsss just to confirm, when removing the <a, should I make sure it s never added in the first place (by changing the js to prevent it), or should I remove it if it already exists in the DOM?

SheikhZaeem · 2025-01-29T11:20:49Z

@Zopsss Hello again. I am facing some problem. I removed "var a = document.createElement("a"), a.setAttribute("href", link) andvanchor.appendChild(a)" from anchors.js folder as I believe the are responsible for generating for the red line image section. However, no changes are seen when I inspect the updated html file in the target folder. Could it be that other js file is adding tag around the image or am I missing something? Thank you in advance.

Zopsss · 2025-01-29T17:21:43Z

@Zopsss just to confirm, when removing the <a, should I make sure it s never added in the first place (by changing the js to prevent it), or should I remove it if it already exists in the DOM?

make sure it is never added in first place

Zopsss · 2025-01-29T17:28:37Z

@Zopsss Hello again. I am facing some problem. I removed "var a = document.createElement("a"), a.setAttribute("href", link) andvanchor.appendChild(a)" from anchors.js folder as I believe the are responsible for generating for the red line image section. However, no changes are seen when I inspect the updated html file in the target folder. Could it be that other js file is adding tag around the image or am I missing something? Thank you in advance.

are you referring to these 2 lines?

checkstyle/src/site/resources/js/anchors.js

Lines 22 to 23 in 977c425

    
           var a = document.createElement("a"); 
        
           a.setAttribute("href", link);

you don't need to remove <a> from all H1 tags, remove it only from red line. These two lines are used to add <a> to all H1 tags.

In the forEach loop check if the current H1 tag is of red line, if yes then do not create <a> tag for it.

How to check if the current H1 tag is of Red Line?

It is under #bannerRight & .pull-right, use JS to identify it.

SheikhZaeem · 2025-01-30T08:59:24Z

@Zopsss Since is already present in target/site/anttask.html, which i am assuming means Maven itself is inserting inside #bannerRight before JavaScript runs. So is coming from

Since the href attribute is present, Maven automatically wraps the image in an tag.

SheikhZaeem · 2025-01-30T09:06:20Z

@Zopsss Also now when red line is pressed, it doesn't do anything. I think makes more sense.

Zopsss · 2025-01-30T09:12:08Z

Github, generate site

github-actions · 2025-01-30T09:15:59Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ee9cbbc_20250130091421/index.html

Zopsss · 2025-01-30T09:26:06Z

Hmm now the line isnt a link anymore but when hovered over, the anchor image appears next to it

I think you can fix this by adding some JS here:

checkstyle/src/site/resources/js/anchors.js

Lines 17 to 20 in 977c425

    
           var anchors = document.querySelectorAll("h1, h2"); 
        
           [].forEach.call(anchors, function (anchorItem) { 
        
               var name = anchorItem.childNodes[0].textContent.replaceAll(" ", "_"); 
        
               var link = "" + url + "#" + name + "";

Add some logic like if the h1 tag is child of #bannerRight then skip that element. This will help you: https://www.w3schools.com/jsref/prop_node_parentelement.asp

Zopsss

fixing extra margins and paddings

Zopsss · 2025-01-30T09:27:27Z

 h1 {
  text-transform: capitalize;
  font-size: x-large;
+  margin: 0px;


revert this, we want margin-top: 0 only for Red Line's H1 tag. Following CSS should remove margin-top and extra padding-top

#bannerRight h1 { margin: 0; } .container-fluid-top { padding-top: 0 !important; }

add this code before following line:

checkstyle/src/site/resources/css/site.css

Line 50 in 977c425

#banner {

to keep same section's CSS together

Zopsss · 2025-01-30T09:32:31Z

+#bannerRight img {
+  transform: translateY(-12px);
+}
+
 #banner {
  border-bottom: 1px solid #fff;
  background: url("../images/header-background.png") repeat-x scroll 0 0 transparent;
+  transform: translateY(-6px);


revert this, use above mentioned code.

Zopsss · 2025-01-30T09:37:01Z

Fix failing CI

https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=25775&view=logs&j=c902ebb4-c9f8-5f09-4e17-ff78fbbc842e&t=9ca98c81-ff64-58f0-9d03-a23ac1c4a111&l=1351

it is asking you to follow our commit message rules.

SheikhZaeem · 2025-01-30T15:48:04Z

@Zopsss Fixed and reverted the code as you said. But I don't know why I get these errors regarding commit messages.

Zopsss · 2025-01-30T15:49:01Z

Github, generate site

Zopsss · 2025-01-30T15:52:23Z

@Zopsss Fixed and reverted the code as you said. But I don't know why I get these errors regarding commit messages.

Read the commit message carefully, it has mentioned some rules regarding commit message. You seem to have break 1st rule. The commit message should contain this prefix: Issue #YourIssueNumber

Normally you should run mvn clean verify before pushing changes, it'll give you such errors so you can fix them early. But this was website issue so it's okay to not run this command.

github-actions · 2025-01-30T15:52:46Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9309e00_20250130155111/index.html

Zopsss · 2025-01-30T15:53:55Z

Please squash all commits. Always keep 1 commit per PR.

SheikhZaeem · 2025-01-30T19:18:25Z

Please squash all commits. Always keep 1 commit per PR.

Can you explain in more detail. Do you mean if i keep changing the code of my pull request again and again, I don't need to write new commit messages?

Zopsss · 2025-01-31T16:11:41Z

Yes, you can amend your changes in your previous commit. No need to create different commit each time. You currently have more than one commit so squash them into one commit, you can google how to do it.

SheikhZaeem · 2025-01-31T22:05:36Z

github, generate site

github-actions · 2025-01-31T22:09:27Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9b2e89c_20250131220758/index.html

…updated anchor links

SheikhZaeem · 2025-01-31T22:45:18Z

github, generate site

SheikhZaeem · 2025-01-31T22:46:17Z

@Zopsss I squashed and then I lost some of the changes i did due to my mistake. But I think now it is fine.

Zopsss · 2025-02-01T05:38:58Z

Github, generate site

github-actions · 2025-02-01T05:45:34Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5d4d484_20250201054442/index.html

SheikhZaeem · 2025-02-01T08:55:01Z

Hey @Zopsss. Thanks for so much help. Is there anything else left for this PR? If not, then I will move to other issues?

Zopsss

Red line isn't a link anymore and it is sticked to the top now. Thanks a lot for all the hard work!

Zopsss · 2025-02-01T16:56:46Z

@romani the Checkstyle logo in banner is a link. But I guess it is fine, or do you want to change it?

romani

@SheikhZaeem , @Zopsss , thanks a lot for collaboration to make a fix !!!

great job !!

Zopsss suggested changes Jan 29, 2025

View reviewed changes

Zopsss suggested changes Jan 30, 2025

View reviewed changes

SheikhZaeem force-pushed the fix-header-image branch 2 times, most recently from 1858a32 to 9b2e89c Compare January 31, 2025 22:01

Issue #16243: Fixed header image issues removed unnecessary tags and …

5d4d484

…updated anchor links

SheikhZaeem force-pushed the fix-header-image branch from 9b2e89c to 5d4d484 Compare January 31, 2025 22:44

Zopsss approved these changes Feb 1, 2025

View reviewed changes

romani approved these changes Feb 1, 2025

View reviewed changes

romani merged commit 42174f8 into checkstyle:master Feb 1, 2025

SheikhZaeem deleted the fix-header-image branch February 2, 2025 14:37

Uh oh!

Conversation

SheikhZaeem commented Jan 28, 2025

Uh oh!

Zopsss commented Jan 29, 2025

Uh oh!

github-actions Bot commented Jan 29, 2025

Uh oh!

Zopsss Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Zopsss commented Jan 29, 2025

Uh oh!

romani commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SheikhZaeem commented Jan 29, 2025

Uh oh!

Zopsss commented Jan 29, 2025

Uh oh!

SheikhZaeem commented Jan 29, 2025

Uh oh!

SheikhZaeem commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SheikhZaeem commented Jan 29, 2025

Uh oh!

Zopsss commented Jan 29, 2025

Uh oh!

Zopsss commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SheikhZaeem commented Jan 30, 2025

Uh oh!

SheikhZaeem commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 30, 2025

Uh oh!

github-actions Bot commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zopsss left a comment

Choose a reason for hiding this comment

Uh oh!

Zopsss Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Zopsss Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Zopsss Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Zopsss commented Jan 30, 2025

Uh oh!

SheikhZaeem commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 30, 2025

Uh oh!

github-actions Bot commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 30, 2025

Uh oh!

SheikhZaeem commented Jan 30, 2025

Uh oh!

Zopsss commented Jan 31, 2025

Uh oh!

SheikhZaeem commented Jan 31, 2025

Uh oh!

github-actions Bot commented Jan 31, 2025

Uh oh!

SheikhZaeem commented Jan 31, 2025

Uh oh!

SheikhZaeem commented Jan 31, 2025

Uh oh!

Zopsss commented Feb 1, 2025

romani commented Jan 29, 2025 •

edited

Loading

SheikhZaeem commented Jan 29, 2025 •

edited

Loading

Zopsss commented Jan 29, 2025 •

edited

Loading

Zopsss commented Jan 30, 2025 •

edited

Loading