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

Star glyph not rendering correctly #4532

Closed
wants to merge 2 commits into from
Closed

Conversation

Wqrld
Copy link

@Wqrld Wqrld commented Jul 28, 2018

fix #4515

good fix for  go-gitea#4515 this time, im not that used to less so i hope this is a good place to put it.
@Wqrld
Copy link
Author

Wqrld commented Jul 28, 2018

failing with sh: write error: Resource temporarily unavailable, any idea why?

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 28, 2018
@Wqrld
Copy link
Author

Wqrld commented Jul 28, 2018

Fixed CI

@Wqrld
Copy link
Author

Wqrld commented Jul 28, 2018

CI failed on mysql but that seems to be just a cloudfront outage.

EDIT: packagelock seems to have changed, is that a problem?

@@ -9,10 +9,10 @@
"dev": true,
Copy link
Member

Choose a reason for hiding this comment

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

package lock updates aren't needed to change the CSS

Copy link
Author

Choose a reason for hiding this comment

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

yes, this hapenned when npm installing less

Copy link
Member

Choose a reason for hiding this comment

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

with git you can:

git add public/css/index.css
git add  public/less/_repository.less
git commit

that will only add specific files to your commit.

@techknowlogick techknowlogick changed the title really fix #4515 Star glyph not rendering correctly Jul 28, 2018
@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Jul 28, 2018
@techknowlogick techknowlogick added this to the 1.6.0 milestone Jul 28, 2018
@techknowlogick
Copy link
Member

CI restarted

@codecov-io
Copy link

Codecov Report

Merging #4532 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4532   +/-   ##
======================================
  Coverage    20.2%   20.2%           
======================================
  Files         156     156           
  Lines       31140   31140           
======================================
  Hits         6292    6292           
  Misses      23890   23890           
  Partials      958     958

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a74426d...f3576fb. Read the comment docs.

@lafriks
Copy link
Member

lafriks commented Jul 28, 2018

I don't think that is correct way to fix this. I'm on mobile can't check right now but fa class should set correct font family already

@exside
Copy link

exside commented Jul 29, 2018

@lafriks agreed, but there is some strange interference between the Semantic UI iconfont and fontawesome...guess that's causing all the trouble.

@atom0s
Copy link

atom0s commented Jul 29, 2018

Simply changing the icon type from fa-star-o to fa-star fixes this rather than a bunch of file edits.

@techknowlogick
Copy link
Member

@atom0s sadly that change also isn't entirely effective, as it would break logic: <i class="icon fa-star{{if not $.IsStaringRepo}}-o{{end}}"></i> is the existing logic, and changing it would mean that a user couldn't tell if they are have starred the repo yet.

@lafriks
Copy link
Member

lafriks commented Jul 30, 2018

Does not changing to <i class="icon fa fa-star{{if not $.IsStaringRepo}}-o{{end}}"></i> help?

@techknowlogick
Copy link
Member

@lafriks that specific change didn't work, however I found replacing icon with fa worked. Even though it did work I'm still unsure about it because icon is used everywhere else in the application.

@lafriks
Copy link
Member

lafriks commented Aug 7, 2018

@techknowlogick @Wqrld it is caused by semantic ui update.
This is correct solution for unstarred use: <i class="icon star outline"></i> for starred: <i class="icon star"></i>

So template file should have:

<i class="icon star{{if not $.IsStaringRepo}} outline{{end}}"></i>

@techknowlogick
Copy link
Member

That's great news! I should be waking up in a few hours and can submit a PR with that change, or if you get to it before me I can give it my LG-TM

@lafriks
Copy link
Member

lafriks commented Aug 7, 2018

I have submitted fix, closing this

@lafriks lafriks closed this Aug 7, 2018
@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
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Star glyph not rendering correctly
7 participants