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

Minor UI tweaks #5980

Merged
merged 34 commits into from Feb 19, 2019

Conversation

9 participants
@jolheiser
Copy link
Member

jolheiser commented Feb 6, 2019

Behavior

  • templates/repo/create.tmpl – Once you select a license you can no longer deselect it.
  • External issue trackers: The issues tab should have an external-link-alt icon and link directly to the external tracker so that you can see the URL by hovering over it.
  • When you select an issue/pull request the toggle button transitions to open & close buttons, which is not only problematic because it still looks like a toggle button but also doesn't make sense because you are never able to open or close an issue/pull request, it's either open or closed, you only ever have one choice (to toggle the current state). I suggest the transition to be removed and instead display an open or close button next to the toggle button.
    select_issue
  • The above check boxes should not be displayed for repositories where you are not allowed to open or close issues/pull requests.
  • 404 pages of files, issues, pull requests, releases and wiki pages should include the repository header if the repository exists.
  • Reset password should directly tell you when a code is invalid: https://try.gitea.io/user/reset_password?code=x
  • public/js/index.js – When editing the labels of an existing issue (see screenshot), the labels are currently saved with every select / deselect, which just leads to unnecessary spam. The labels should only be saved after closing the popup, which is also the way GitHub does it.
    label_popup

Visuals

  • templates/repo/header.tmpl – Visually disable fork button on own repositories (you cannot clone them) by adding disabled class .
    disable_fork
  • templates/repo/home.tmpl – Give New Pull Request button a text label instead of a cryptic icon.
    pull-button
  • public/less/themes/arc-green.less – The dark theme should use a less vibrant color for green buttons, I suggest #87ab63, the same color used for links.

jolheiser and others added some commits Feb 3, 2019

Remove all CommitStatus when a repo is deleted
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 6, 2019

As this is one of my first more in-depth PRs feel free to give me pointers along the way.

One thing I had a question about was, do the maintainers want all of the changes in one PR, or spread out into 2 (or 3)?

EDIT: Of course, my first run in with go fmt errors. I'll make sure to run the command for it next time I make changes.

@techknowlogick techknowlogick added this to the 1.8.0 milestone Feb 6, 2019

@GiteaBot GiteaBot added the lgtm/need 2 label Feb 6, 2019

@jolheiser jolheiser referenced this pull request Feb 6, 2019

Closed

Minor UI tweaks #5782

0 of 10 tasks complete
@lunny

This comment has been minimized.

Copy link
Member

lunny commented Feb 7, 2019

CI failed

@jolheiser jolheiser referenced this pull request Feb 7, 2019

Closed

UI button clarification #5993

2 of 7 tasks complete
@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Feb 7, 2019

Based on your screenshots, I think your fork is not up to date with some recent changes (button styles), please rebase the branch against master.

Regarding "New Pull Request" button, I'd say also remove the green color, making it plain grey.

@jolheiser jolheiser force-pushed the jolheiser:master branch from 3ec1e87 to d16f404 Feb 9, 2019

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 9, 2019

I feel like the button is a little close, but acceptable.
issue action button

When a user cannot create issues.
issue cannot create

On a repo's home page, the Pull Request button is now in text (and on the other side).
pr home

If not on the home page, start displaying breadcrumb instead of PR button.
breadcrumb else

When a user encounters a 404 on any URL that would otherwise show a repo, the repo's header is displayed.
repo 404

Disabled the fork button if the user cannot fork the repo.
can t fork own

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Feb 9, 2019

Good changes.

  • Maybe add a basic class to the new pull request button. Personally, I find these grey buttons a bit ugly.
  • The checkboxes on issues need vertical centering.

@jolheiser jolheiser force-pushed the jolheiser:master branch from 9062a0a to 65454d2 Feb 9, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 9, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@2982413). Click here to learn what that means.
The diff coverage is 31.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5980   +/-   ##
=========================================
  Coverage          ?   38.85%           
=========================================
  Files             ?      352           
  Lines             ?    50045           
  Branches          ?        0           
=========================================
  Hits              ?    19445           
  Misses            ?    27784           
  Partials          ?     2816
Impacted Files Coverage Δ
routers/user/auth.go 13.07% <0%> (ø)
modules/context/context.go 49.7% <100%> (ø)
modules/context/repo.go 59.29% <25%> (ø)
routers/repo/issue.go 36.42% <42.85%> (ø)

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 2982413...e32153c. Read the comment docs.

jolheiser added some commits Feb 6, 2019

Minor UI tweaks (#5782)
Added 'No License' option
Added link and octicon change for external issue trackers
Reset password now notifies right away if the code is invalid

Signed-off-by: jolheiser <john.olheiser@gmail.com>
More UI tweaks
More info in PR
UI updates
Made the PR button a "basic" button
Vertically centered the issue checkboxes
Labels will update only once after modal is closed

@jolheiser jolheiser force-pushed the jolheiser:master branch from b5777c0 to 477d07b Feb 10, 2019

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 10, 2019

Some new images with @silverwind's suggestions.

The new look of the PR button
pr home new

Issue checkboxes vertically aligned
issue vertical align

Issue labels should only update once after closing the modal. Currently it is storing all actions in a list and then looping over the list, updating the issue meta. It currently relies on the fact that adding a label to an issue that's already added (and vice-versa) doesn't update the label. I'm not sure I really like relying on that, however I thought I'd get second opinions on whether it's worth attempting to keep track of "new" actions for labels vs what they started as.
update labels after modal closes

@jolheiser jolheiser changed the title WIP: Minor UI tweaks Minor UI tweaks Feb 10, 2019

jolheiser added some commits Feb 10, 2019

Commit to reference related issues
Resolves #5782
Resolves #5861
Addresses original question in #5993
@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented Feb 11, 2019

Can you make the new pull request button the same height as the dropdown left to it? Also what about a color for said button?

And you need to regenerate stylesheets.

jolheiser added some commits Feb 12, 2019

Added back distinction between issue actions and filters (overlooked …
…it before)

Moved action button over next to other action dropdowns
@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 12, 2019

@lafriks Sorry about that, I had not understood what that second block was for.
I added it back in and made different changes to re-implement my original change.

Updated screenshot for actions:
gitea issue actions new
I moved the button over with the rest of the actions, let me know if you prefer it to the left like my original change screenshot.

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 13, 2019

Should I add an entry to the i18n file for this, or is there some other default wording that would work?
https://github.com/go-gitea/gitea/pull/5980/files#diff-2e12b4f1d396a426bd40609e02c23f71R78

jolheiser added some commits Feb 13, 2019

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 15, 2019

This PR is ready for review again. 😄

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 19, 2019

Looking at code it looks good, I now just need to test it. One thing I am going to focus on testing is to ensure that for repos 404 it doesn’t show headers to users who don’t have permissions

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 19, 2019

Looking at code it looks good, I now just need to test it. One thing I am going to focus on testing is to ensure that for repos 404 it doesn’t show headers to users who don’t have permissions

That's a good point, I don't think I did anything specific for that, so it would be whether it happens earlier in the context.

EDIT: I wasn't able to get to a private repo if I was not signed in or not the owner. (Of course there are more checks I'm sure you will do, but it's a promising start.)

@0x5c

This comment has been minimized.

Copy link
Contributor

0x5c commented Feb 19, 2019

Just to note that this PR fixes #5861 (#5782 being a meta-issue)

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 19, 2019

In testing 404, I'm getting prompted for basic auth.
screen shot 2019-02-19 at 3 49 31 pm

also when I successfully provide my credentials via basic auth it gives me the standard 404 without a header.

@jolheiser

This comment has been minimized.

Copy link
Member Author

jolheiser commented Feb 19, 2019

What an interesting thing. I cannot reproduce this in Chrome, as it returns a 401.
However, on Edge I was able to reproduce this.
gitea 404

Not sure on the browser side what is happening, but from Gitea's point of view that shouldn't be a valid URL...

Not sure if you want me to dig more into that (if I'm not mistaken, I think it's browser behavior), however testing with a URL similar to http://localhost:9090/tklk/blog/src/branch/master/404 should yield a 404 without a header if no permissions, or with a header if correct permissions.

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Feb 19, 2019

Browsers usually present these dialogs when they see a WWW-Authenticate response header, thought these should usually be only sent on a 401, not a 404.

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 19, 2019

@jolheiser don't worry about it. I was able to replicate being prompted for basic auth in master and so it isn't something that your branch is adding. I have one or two more things I'm still testing, but hopefully I'll be done today.

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 19, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 19, 2019

@techknowlogick techknowlogick merged commit d26d249 into go-gitea:master Feb 19, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment