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

Fix pull request commit list item positions #12382

Closed
wants to merge 1 commit into from
Closed

Fix pull request commit list item positions #12382

wants to merge 1 commit into from

Conversation

jaqra
Copy link
Contributor

@jaqra jaqra commented Jul 30, 2020

I dont know if there is issue related this

Before

before

After

after

@silverwind
Copy link
Member

Is there an example of this on try.gitea.io? Clearfix seems so oldschool, I wonder of there are better solutions.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 30, 2020
@jaqra
Copy link
Contributor Author

jaqra commented Jul 30, 2020

@jaqra
Copy link
Contributor Author

jaqra commented Jul 30, 2020

looks like this happens because of float rules

@jolheiser
Copy link
Member

It gets better with signed commits 😂

https://try.gitea.io/jolheiser/normal/pulls/1

@jaqra
Copy link
Contributor Author

jaqra commented Jul 30, 2020

@jolheiser actually this looks like so artistic. maybe we should NOT fix this 😜

@silverwind
Copy link
Member

silverwind commented Jul 30, 2020

Hmm, not seeing it in either Firefox or Chrome on macOS. Which browsers/OS are you on:

image

@jaqra
Copy link
Contributor Author

jaqra commented Jul 30, 2020

Chrome 84.0.4147.105 (64 bit) / Windows 10

@silverwind
Copy link
Member

silverwind commented Jul 30, 2020

Weird, Chrome 84.0.4147.105 on macOS renders it fine. Such issues are usually not platform-specific.

@silverwind
Copy link
Member

silverwind commented Jul 30, 2020

This change does regress vertical centering on the element for me and also increase element height by 2px. I don't think it's right. Ideally, I'd like to see these floats converted to flexbox but it will require some markup changes as well I imagine.

Before:
image

After:
image

@mrsdizzie
Copy link
Member

Interesting -- can't reproduce this either on macOS in any browser

@lunny
Copy link
Member

lunny commented Jul 31, 2020

Have you customerized your gitea's css?

@jolheiser
Copy link
Member

Something interesting to note, checking now on my Linux machine, same browser as before (Brave), it looks just fine.
My earlier test was on Windows.

@jaqra
Copy link
Contributor Author

jaqra commented Jul 31, 2020

@lunny i did not customize gitea's css. i can reproduce also in try

this should be chrome's issue for windows build. because there is one more thing https://gitea.com/gitea/changelog/pulls/44

Untitled

@silverwind
Copy link
Member

I can try on Windows later but ultimately I think we should flexbox these timeline messages instead of adding extending the float hacks further.

@jaqra
Copy link
Contributor Author

jaqra commented Jul 31, 2020

@silverwind I dont know best practices for flex boxes but i will try

@silverwind
Copy link
Member

silverwind commented Jul 31, 2020

Basically you want a horizontal flexbox with spacing between, e.g.

<div class="timeline-entry">
  <div class="timeline-entry-left"></div>
  <div class="timeline-entry-right"></div>
</div>
.timeline-entry {
  display: flex;
  justify-content: space-between;
  align-items: center;
}

.timeline-entry-left,
.timeline-entry-right {
  display: flex;
  align-items: center;
}

This should give a basic layout that works. Margins will need to be added because inside a flexbox, HTML whitespace is not significant. CSS-based text truncation may be needed on the left element. Or if the server already truncates, that's fine too.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 31, 2020

Can not reproduce on Chromium 84.0.4147.89 on Windows

@jaqra
Copy link
Contributor Author

jaqra commented Jul 31, 2020

@CirnoT this should be new in Chrome 84.0.4147.105

@silverwind
Copy link
Member

Maybe report it to https://bugs.chromium.org/p/chromium/issues.

@zeripath
Copy link
Contributor

zeripath commented Jul 31, 2020

Just to say I can't reproduce on: Chrome linux 84.0.4147.105 (Official Build) (64-bit)

@silverwind
Copy link
Member

silverwind commented Jul 31, 2020

If no one can reproduce, maybe it's a extension or something interferring? Tried a clean browser profile yet?

@jaqra
Copy link
Contributor Author

jaqra commented Jul 31, 2020

I tried on clean browser. Is there anyone using windows 10 google chrome 84.0.4147.105

@silverwind
Copy link
Member

Just tried that exact browser/os and still cannot reproduce. Version 84.0.4147.105 (Official Build) (64-bit). Update graphics drivers perhaps?

@jaqra
Copy link
Contributor Author

jaqra commented Jul 31, 2020

@silverwind i have 2 laptops. I can reproduce on both

@CirnoT
Copy link
Contributor

CirnoT commented Aug 1, 2020

@jaqra 84.0.4147.105 a6b12dfad6663f13a7e16e9a42a6a4975374096b-refs/branch-heads/4147@{#943} (Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.105 Safari/537.36), can not reproduce. If this started happening after updating to .105 then I strongly advise opening bug report with Chromium, breaking changes in CSS are not supposed to happen in patch releases for Chromium.

@jaqra
Copy link
Contributor Author

jaqra commented Aug 1, 2020

@lunny i did not customize gitea's css. i can reproduce also in try

this should be chrome's issue for windows build. because there is one more thing https://gitea.com/gitea/changelog/pulls/44

Untitled

Interestingly this looks solved. This is not broken now

image

But commit positions is still broken

image

Feel free to close this pr if no one can reproduce

@silverwind
Copy link
Member

silverwind commented Aug 1, 2020

The misaligned reactions may be an effect of #12317 but I did not observe it. It certainly made them a bit smaller (1.25em vs previous 1.5em)

@jaqra
Copy link
Contributor Author

jaqra commented Aug 1, 2020

@silverwind this screenshots are from gitea.com. yesterday and today. i guess gitea.com did not upgrade

@silverwind
Copy link
Member

gitea.com is at e67c04251 so should have that PR.

@stale
Copy link

stale bot commented Oct 4, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Oct 4, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Nov 22, 2020

This has started happening for me recently, don't really feel like bisecting which commit broke it explicitly, but the actual reason for this happening is that the spacing between two singular-commit elements is too low and the icon itself is bigger than it looks (it has invisible background) causing it to overflow.

@lunny
Copy link
Member

lunny commented Nov 23, 2020

This should have been resolved.

@jaqra
Copy link
Contributor Author

jaqra commented Nov 23, 2020

looks problem resolved. i am closing this

@jaqra jaqra closed this Nov 23, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants