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 fork button #6223

Merged
merged 10 commits into from
Mar 6, 2019
Merged

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Mar 1, 2019

Fixes #6222

When disabling only the first part, I didn't like how semantic displayed it.
fork_basic

So instead I moved the basic class into an if-else.
It appears basic when a user can fork (no change from existing style)
It appears normal (but disabled) if they cannot fork. (image below)
fork_fix

There was also the case of a non-signed in user, which is why this is a separate if-else within the element to avoid any doubled up classes in favor of another logic flow.

@jolheiser jolheiser changed the title 6222 fix fork button Fix fork button Mar 1, 2019
@codecov-io
Copy link

codecov-io commented Mar 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@6460cff). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6223   +/-   ##
=========================================
  Coverage          ?   38.83%           
=========================================
  Files             ?      355           
  Lines             ?    50256           
  Branches          ?        0           
=========================================
  Hits              ?    19518           
  Misses            ?    27909           
  Partials          ?     2829

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 6460cff...673fa03. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 1, 2019
@GiteaBot GiteaBot 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 Mar 1, 2019
@techknowlogick techknowlogick added this to the 1.8.0 milestone Mar 1, 2019
@techknowlogick techknowlogick added type/bug topic/ui Change the appearance of the Gitea UI labels Mar 1, 2019
@silverwind
Copy link
Member

silverwind commented Mar 2, 2019

Still looks kind of weird, I'd fix the disabled CSS to use something like opacity: .5 on the whole button. Plus points for also adding cursor: not-allowed.

@silverwind
Copy link
Member

Also, I think some manual styling to make border-right change on hover would be nice.

@jolheiser
Copy link
Member Author

After messing with CSS for a bit, this is what I came up with. Note that the right-button to get to forks is still available and the cursor is a pointer like normal.
It adds the cursor: not-allowed; change as well to the left-button. Since I needed to manually counteract any prior hover event styling, there's a chance I missed something that would show up differently in another browser.
Please let me know what further changes, if any, are needed.

fork-disabled

@silverwind
Copy link
Member

silverwind commented Mar 2, 2019

I'd change the disabled class to something a bit more more generic like .disabled-repo-button so it could be used on the other buttons too. Or even go full out on overriding the .disabled class. pointer-events: none might be required to actually make the button unclickable.

Some lightly tested suggestions (the margin change on the label makes border-right on the left button show). I'm sure more improvements are possible (the color/box-shadow change seems unneccessary):

.repo-buttons .disabled-repo-button .label {
  opacity: .5;
}

.repo-buttons .disabled-repo-button a.button {
  opacity: .5;
  cursor: not-allowed;
  pointer-events: none;
}

.repo-buttons .disabled-repo-button a.button:hover {
  background: none !important;
  color: rgba(0,0,0,.6) !important;
  box-shadow: 0 0 0 1px rgba(34,36,38,.15) inset !important;
}

.repo-buttons .button > .label {
  border-left: none !important;
  margin: 0 !important;
}

Signed-off-by: jolheiser <[email protected]>
@jolheiser
Copy link
Member Author

I left pointer-events on because otherwise you lose the cursor and tooltip.
Clicking on it didn't seem to do anything in Chromium (which is intended behavior), however at the moment I cannot test on other browsers.

@silverwind
Copy link
Member

silverwind commented Mar 3, 2019

Almost there, but we need to modify the last style to make the inner border show:

.repo-buttons .ui.labeled.button > .label {
  margin: 0 !important;
}

Reason being the !important in Semantic here:

screenshot 2019-03-03 at 17 11 04

Signed-off-by: jolheiser <[email protected]>
@jolheiser
Copy link
Member Author

jolheiser commented Mar 3, 2019

Okay, updated. Thanks for the input, @silverwind
I can make UI changes, but I am definitely not a styling expert. 😅

@hexfs
Copy link

hexfs commented Mar 5, 2019

Conflict

@jolheiser
Copy link
Member Author

Conflict

Resolved

@hexfs
Copy link

hexfs commented Mar 5, 2019

@techknowlogick GTM

@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 Mar 6, 2019
@techknowlogick techknowlogick merged commit 608781f into go-gitea:master Mar 6, 2019
@jolheiser jolheiser deleted the 6222_fix_fork_button branch March 14, 2019 16:06
@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/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 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot go to my fork when on main org project
7 participants