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

[JENKINS-66749] Missing spaces in upstream/downstream project view #5850

Merged
merged 3 commits into from
Nov 5, 2021

Conversation

jamesD33064
Copy link
Contributor

@jamesD33064 jamesD33064 commented Oct 24, 2021

Missing spaces in upstream/downstream project view

截圖 2021-10-24 下午6 08 46

See JENKINS-66749.

Proposed changelog entries

  • Add space between icon and project name in upstream & downstream section in project page.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

I add 5px space between icon and project name
@MarkEWaite MarkEWaite changed the title 66749 "Missing spaces in upstream/downstream project view" [JENKINS-66749] Missing spaces in upstream/downstream project view Oct 26, 2021
@timja timja added web-ui The PR includes WebUI changes which may need special expertise bug For changelog: Minor bug. Will be listed after features labels Oct 26, 2021
timja
timja previously approved these changes Oct 26, 2021
@timja timja requested review from uhafner and a team October 26, 2021 07:09
@timja timja dismissed their stale review October 26, 2021 08:56

what was I thinking

@timja timja added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Oct 27, 2021
@jamesD33064
Copy link
Contributor Author

截圖 2021-10-28 上午11 39 32

It's ineffective

@timja timja requested a review from uhafner October 28, 2021 06:30
@uhafner
Copy link
Member

uhafner commented Oct 28, 2021

Hmm, I modified the DOM in the page and it works:

Bildschirmfoto 2021-10-28 um 09 09 34

Is there something in our jelly processing that behaves differently? What is the DOM in your example?

@uhafner
Copy link
Member

uhafner commented Oct 28, 2021

And what Jenkins version are you using?

@jamesD33064
Copy link
Contributor Author

My Jenkins version is 2.315
截圖 2021-10-28 下午10 21 05
5

@timja
Copy link
Member

timja commented Oct 28, 2021

Can you put the icon and breakable on the same line and then put a space in between them?

@jamesD33064
Copy link
Contributor Author

截圖 2021-10-31 下午2 56 21

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

The link also needed a margin, based on the fix in #5507 (I've pushed a commit to get this over the line)

I also created a css class for it, not the best naming but wasn't sure what do call it.

Tested this and it works

@timja timja removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Oct 31, 2021
@timja timja requested a review from a team October 31, 2021 16:30
@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 1, 2021
@timja
Copy link
Member

timja commented Nov 1, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@uhafner
Copy link
Member

uhafner commented Nov 1, 2021

I think a similar fix is required for buildLink.jelly. I'm currently trying to refactor my summary.jelly file with the new icons and here the gap is also missing.

Additionally, the icon seems to render not correctly aligned vertically. Any idea what needs to be fixed in the CSS here? The first pixel seems to be aligned with the top of the text, it would look better if it is vertically centered?

Bildschirmfoto 2021-11-01 um 15 21 27

@timja
Copy link
Member

timja commented Nov 1, 2021

Will try take a look later on

@uhafner
Copy link
Member

uhafner commented Nov 1, 2021

I played around a little bit and it actually seems that our vertical alignment is broken somehow for the build status. Even on the same page I get different offsets for the alignment of text and SVG icon:

Bildschirmfoto 2021-11-01 um 15 51 47

Here the icon for the Java and Error Prone warnings are 4 pixels above the text. For the same icon for the Checkstyle and SpotBugs summary the icon starts just 1 pixel above the text. I have no idea what causes this misalignment?

(Sorry this is unrelated to this PR, simply an observation while trying to use the icon in my summary.jelly)

@timja timja merged commit 322aac3 into jenkinsci:master Nov 5, 2021
@uhafner
Copy link
Member

uhafner commented Nov 5, 2021

Will try take a look later on

Hmm, this is not yet solved? I think this problem occurs in several (all?) places where we use the icon.

@timja
Copy link
Member

timja commented Nov 5, 2021

Will try take a look later on

Hmm, this is not yet solved? I think this problem occurs in several (all?) places where we use the icon.

Haven't had a chance, but this fixes this specific instance no reason to hold it back, we can iterate on improvements

uhafner added a commit to uhafner/jenkins that referenced this pull request Nov 5, 2021
uhafner added a commit to uhafner/jenkins that referenced this pull request Nov 5, 2021
@uhafner
Copy link
Member

uhafner commented Nov 5, 2021

Ok, this just needs additional and duplicate work, I created a followup: #5887.

@timja
Copy link
Member

timja commented Nov 5, 2021

Thanks for sorting it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants