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

Implementation of all repositories of a user from user->settings #1740

Merged
merged 6 commits into from
Sep 14, 2017

Conversation

DblK
Copy link
Member

@DblK DblK commented May 17, 2017

This PR implement the display of all repository owned by the user in its settings.

Here is the UI:

The menu:
image

With no repository:
image

With all kind of repositories:
image

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 17, 2017
@lunny lunny added this to the 1.3.0 milestone May 17, 2017
@@ -429,6 +430,8 @@ remove_account_link = Remove linked account
remove_account_link_desc = Delete this account link will remove all related access for your account. Do you want to continue?
remove_account_link_success = Account link has been removed successfully!

repos_none = You do not owned any repository
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrasing on this is a bit awkward, "You do not own any repositories" is more correct. Although I'm not sure using "own" here is the right term either.

Copy link
Member Author

Choose a reason for hiding this comment

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

"You do not have any repositories", maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, that sounds a lot better in my opinion. Maybe even "You do not currently have any repositories". 😃

@@ -430,7 +430,7 @@ remove_account_link = Remove linked account
remove_account_link_desc = Delete this account link will remove all related access for your account. Do you want to continue?
remove_account_link_success = Account link has been removed successfully!

repos_none = You do not owned any repository
repos_none = You do not currently have any repositories
Copy link
Contributor

@pgaskin pgaskin May 17, 2017

Choose a reason for hiding this comment

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

That's a bit wordy and is not consistent with the rest of the strings. I think it should be "You do not own any repositories".

Copy link
Contributor

Choose a reason for hiding this comment

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

I commented on this earlier and I don't like the wording where you own repositories, I get the meaning behind it but don't think it's too accurate.

Copy link
Contributor

Choose a reason for hiding this comment

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

All the other strings refer to it as owning repositories. Consistency makes everything easier to use and look like everything fits together. What do you mean by it isn't too accurate.

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM :)

@@ -272,6 +272,7 @@ ssh_keys=Clés SSH
social=Réseaux Sociaux
applications=Applications
orgs=Organisations
repos=Dépôts
Copy link
Member

Choose a reason for hiding this comment

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

This should be done via CrowdIn. (all languages other than English)

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the french translation

@bkcsoft
Copy link
Member

bkcsoft commented May 21, 2017

Looks good 🙂 why the ad-hoc list though? It differs from all other lists (tables) in the App. Looks very "thrown together" and out of place 😕

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

DblK commented May 22, 2017

You are right. I didn't know how to display it. It was the same as Github was before.
Here is the new UI I've done for this:
image

L-G-T-M does not seems to see your last comments.
Commit and push will be done pretty soon ;)

@strk
Copy link
Member

strk commented May 28, 2017

LGTM - but it would be great if you could add integration tests for this new endpoint.

@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 May 28, 2017
@pgaskin
Copy link
Contributor

pgaskin commented Jun 16, 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 Jun 16, 2017
@lunny
Copy link
Member

lunny commented Jun 16, 2017

@geek1011 so many conflicted files?

@pgaskin
Copy link
Contributor

pgaskin commented Jun 16, 2017

@lunny I was saying L-G-T-M about the idea, and the fact that it works when that branch is built, not the conflicts when merging it.

@bkcsoft
Copy link
Member

bkcsoft commented Jun 19, 2017

fixed merge-conflicts

@lunny
Copy link
Member

lunny commented Jun 23, 2017

It seems there is a line break after the icon?
untitled

@lunny
Copy link
Member

lunny commented Jun 23, 2017

@DblK Please confirm the problem, maybe we could move this to v1.2.

@pgaskin
Copy link
Contributor

pgaskin commented Jun 23, 2017

@lunny @DblK Yes, there is a problem.

It can be fixed by making the icon and content inline-block, instead of floating it. Floating is less predictable, and it does not work because the content div fills as much space as possible.

@DblK
Copy link
Member Author

DblK commented Jul 4, 2017

I'll fix it soon I hope kinda busy right now.

@lunny
Copy link
Member

lunny commented Aug 28, 2017

This could be merged after the little problem resolved. @DblK maybe some maintainer could help you if you are busy now.

@DblK
Copy link
Member Author

DblK commented Sep 9, 2017

If someone else cqn finish it go for it. Not much time right now.

@lunny
Copy link
Member

lunny commented Sep 14, 2017

rebased and see below screenshot.
-1

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1740   +/-   ##
========================================
  Coverage          ?   27.7%           
========================================
  Files             ?      83           
  Lines             ?   16914           
  Branches          ?       0           
========================================
  Hits              ?    4686           
  Misses            ?   11553           
  Partials          ?     675
Impacted Files Coverage Δ
routers/user/setting.go 0% <0%> (ø)

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 1739e84...c3146b3. Read the comment docs.

@lafriks lafriks merged commit e5d80b7 into go-gitea:master Sep 14, 2017
@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/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants