Skip to content

Conversation

@sapk
Copy link
Member

@sapk sapk commented Apr 6, 2017

@Morlinest detected in #1418 a bug introduced in #1389.

This is mainly cause because I didn't catch impact on hooks pages and sub .menu item of .list

For .menu I use > parent link to fix sub .item catch.
And for hook remove specific code that interfere with global style.

This is a quick and "simple" fix and more work can be done to use less css hack and use class from semantic like .divided in place of :not(:first-child). This is done in #1450 and a progress is allready done in #1435 by @Morlinest

Menu

before
before-menu

after
after-menu

Hooks pages

before
before-githook
before-webhook
before-hook-history-1

hook history details

before-hook-history-2

before-hook-history-3



after
after-githook
after-webhook
after-hook-history-1

hook history details

after-hook-history-2

after-hook-history-3


For verification of no regression, I add screens of related pages of initial PR #1389.

settting-app
settting-ssh
settting-email
repo-deploy-key

@lunny
Copy link
Member

lunny commented Apr 7, 2017

So what's the different this from #1435 and #1450 ?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 7, 2017
@sapk
Copy link
Member Author

sapk commented Apr 7, 2017

#1435 just fix branch by changing aspect because before the have had a update button as large as the page. That what king of "ugly" and by changing that also fix by removing use of .list (that useless because not a list)

#1450 Is going further by removing a other css hack :not(:first-child) that apply top border for list but that useless because semantic have a .divided class that does the same. But that imply to check for all .settings pages that include a .list. Implementing this would also fix the bug that I introduce.

This PR is more of a simple&quick fix that ready to be apply.

@lunny lunny added this to the 1.2.0 milestone Apr 7, 2017
@lunny lunny added the type/bug label Apr 7, 2017
@lunny
Copy link
Member

lunny commented Apr 7, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 7, 2017
@appleboy
Copy link
Member

appleboy commented Apr 7, 2017

LGTM

@tboerger tboerger 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 Apr 7, 2017
@appleboy appleboy merged commit d9db188 into go-gitea:master Apr 7, 2017
@sapk sapk deleted the fix-ui-1418-bug branch April 28, 2017 10:03
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants