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 alignment for commits on dashboard #11595

Merged
merged 7 commits into from
May 27, 2020

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented May 24, 2020

Fix allignment issue between commit SHA and commit message

Old:
Screen Shot 2020-05-24 at 1 01 44 AM

New:
Screen Shot 2020-05-25 at 10 08 22 PM

Fix allignment issue between commit SHA and commit message
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 24, 2020
@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label May 24, 2020
@zeripath zeripath added this to the 1.13.0 milestone May 24, 2020
@zeripath
Copy link
Contributor

Hmm this seems to make the text jump for me here:

Screenshot from 2020-05-24 08-49-00

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

doesn't appear to work

@mrsdizzie
Copy link
Member Author

@zeripath hmm weird -- does the dashboard on gitea.com look ok to you? Right now for me everything (which is only mirror synching) looks broken like my first screenshot. This change makes them all look normal for me in every browser.

Do you see them as broken but this change doesn't fix it?

@zeripath
Copy link
Contributor

No the gitea.com dashboard looks broken too.

Fiddling a little the issue might be that the SHA link isn't display: inline-block

@zeripath
Copy link
Contributor

Sorry or rather that the text.truncate is display: inline-block

@CirnoT
Copy link
Contributor

CirnoT commented May 24, 2020

Does not work here either, it's broken with and without this PR, just in different direction. Changing it to -4px works for me but that's a rather poor hack that most likely depend on font too.

@mrsdizzie
Copy link
Member Author

Weird, must be some combination of font/os that gives different results it looks totally fine in all 3 browsers on MacOs with this PR but I just tried in a windows vm and see the same thing as @zeripath .

The fact that its a negative margin makes me feel like its making up for something else that needs to be fixed, though unsetting display: inline-block makes it look mostly better, and causes margin top and bottom to just have no effect. WIll need to look into more

@silverwind
Copy link
Member

Remove all hacky margins on img and span and set <li> to display: flex; align-items: center:

image

The flexbox makes the whitespace collapse so you'll need something like li > * + * {margin-left: .35rem} to restore it:

image

@silverwind
Copy link
Member

Also, I think you can add small top/bottom margins to <li> so multiple messages are not stuffed together so tightly, e.g. .125rem on both sides.

@mrsdizzie
Copy link
Member Author

@silverwind thanks, applied those changes and seems to work better in all places now

@mrsdizzie mrsdizzie requested a review from zeripath May 26, 2020 18:36
@zeripath
Copy link
Contributor

Screenshot from 2020-05-26 20-17-43

@zeripath
Copy link
Contributor

seems not unfortunately

@CirnoT
Copy link
Contributor

CirnoT commented May 26, 2020

Works for me @zeripath

chrome_2020-05-27_01-55-00
chrome_2020-05-27_01-55-14

@silverwind
Copy link
Member

Works for me too:

image

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 27, 2020
@mrsdizzie
Copy link
Member Author

@zeripath you use Ubuntu right? I set up a vm and think this is an issue with Liberation Mono font which was the font it chose with default settings

It seems to just size much different than others. Removing it from the mono font stack fixes it, but obviously a problem if that is going to be the default font selected for many users. Not really sure what would be a good way of working with that:

May-26-2020 20-18-49

@silverwind
Copy link
Member

silverwind commented May 27, 2020

If Liberation Mono is causing issues, maybe we want to move it after all other monospace fonts, right before monospace. Thought I'm not sure if any better suitable fonts are present by default on Linux systems but I think Consolas might be commonplace.

@mrsdizzie
Copy link
Member Author

In this test for Ubuntu it falls back to 'monospace' when removing Liberation Mono which seems to then use DejaVu Sans Mono from the system which looks better imo. Not sure what other distributions have installed other than Liberation Mono but I can't imagine there is only one monospace font installed.

@silverwind
Copy link
Member

Adding DejaVu Sans Mono before Liberation Mono sounds like a good idea, if it works better.

@silverwind
Copy link
Member

silverwind commented May 27, 2020

That baseline change does move the hash and message 1px up for me, I think I prefer the previous center and fix the fonts.

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

Fix the fonts is not a proper solution; you can try manipulate list to look fine to you but in the end you can never be sure what user has installed and what browser will use. Aligning to baseline seems like the only option where you can be sure that different fonts will be aligned to each other visually.

@silverwind
Copy link
Member

silverwind commented May 27, 2020

I'd say the behaviour in #11595 (comment) is a font/browser bug that should not require us to make specific workarounds for as these may bite us in the long run (after the font/browser is fixed).

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

Yes on Libertion Mono the text is around 1px too high compared to avatar, but that's because the font's baseline is off. My proposed changes do not workaround the font issue, they align items to their baseline. This looks off on Libertion Mono for same reason the original solution did, but at least it's less visible now. Other fonts look fine to me and text is aligned perfectly to avatar.

chrome_2020-05-27_11-07-15

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

In the end, baseline is the only thing that is guaranteed to visually align two different font types and I consider it the proper solution long-term.

Here's better picture without rounded corners which might bend how its visually percepted.
chrome_2020-05-27_11-13-30

@silverwind
Copy link
Member

The alignment looks ok in those screenshots. What I don't quite get is how align-self: flex-end; on the <img> fixes the image. Care to explain?

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

Good question, I have no idea why it actually visually centers image.

Copy link
Contributor

@CirnoT CirnoT left a comment

Choose a reason for hiding this comment

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

Try with this and

        .push.news .content ul {
            line-height: 18px;

margin-bottom: .5rem;

img {
align-self: flex-end;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
align-self: flex-end;
align-self: flex-start;

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

There does indeed seem to be some misalignment when zoomed out
chrome_2020-05-27_12-09-37

The new proposed changes (flex-start and line-height) seem to eliminate it at the cost of putting the image slightly down when zoomed in (same as on GitHub), tho interestingly it still seems visually centered despite that.

@zeripath
Copy link
Contributor

it's possibly font x-height differences between the monospace font and the sans-serif font used for the message.

The solution might simply be to stick the sha in side a rounded-box like we do elsewhere then the jarring font change would be less visible.

@CirnoT
Copy link
Contributor

CirnoT commented May 27, 2020

The new proposed changes seem to work rather well for me.
The zoom-in is on 500%.

Default font on Windows 10:
chrome_2020-05-27_12-16-53
chrome_2020-05-27_12-17-14

Liberation Mono:
chrome_2020-05-27_12-17-43
chrome_2020-05-27_12-17-32


For reference, GitHub:
chrome_2020-05-27_12-18-26
chrome_2020-05-27_12-18-41

@mrsdizzie
Copy link
Member Author

@CirnoT applied those changes, looks ok to me on various systems/fonts

@zeripath I think one of my least favorite parts of Gitea UI is the SHA1 inside of a rounded box : ( I'd rather see those all eventually get replaced rather than add another one here. These changes seem to make it work and look as it is intended to look now

@zeripath zeripath merged commit 5d78792 into go-gitea:master May 27, 2020
@mrsdizzie
Copy link
Member Author

I think this should be backported to 1.12 -- I'm not sure exactly where but the bug this fixes seems like it was introduced somewhere in 1.12 (probably one of the larger timeline UI changes). I don't see it on 1.11

@zeripath
Copy link
Contributor

Please send backport

mrsdizzie added a commit to mrsdizzie/gitea that referenced this pull request May 29, 2020
Fix alignment issue between commit SHA and commit message
@zeripath zeripath added the backport/done All backports for this PR have been created label May 29, 2020
lafriks pushed a commit that referenced this pull request May 30, 2020
Fix alignment issue between commit SHA and commit message
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
Fix alignment issue between commit SHA and commit message
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants