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

Use custom search for each filter type in dashboard #2343

Merged

Conversation

Morlinest
Copy link
Member

@Morlinest Morlinest commented Aug 20, 2017

Based on #2326 (need to be merged first)
Discussion on #2312

Fast network (no restrictions):
2017-11-01_16-26-45

Slow network ("Slow 3G" through Chrome network setting):
2017-11-01_16-24-56

Edit: Updated screnshots/gifs.

@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch from 4278a1b to aa6a2c9 Compare August 20, 2017 13:58
@lafriks lafriks added type/bug type/enhancement An improvement of existing functionality and removed type/bug labels Aug 20, 2017
@lafriks lafriks added this to the 1.x.x milestone Aug 20, 2017
@@ -110,11 +110,37 @@ type SearchRepoOptions struct {
// maximum: setting.ExplorePagingNum
// in: query
PageSize int `json:"limit"` // Can be smaller than or equal to setting.ExplorePagingNum
// Type of repository to search
//
// in: query
Copy link
Member

Choose a reason for hiding this comment

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

Could you please run make generate-swagger to regenerate the swagger spec? This might resolve the conflict. If not please resolve the conflicts manually.

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 did. Conflicts are new due to new master. I'll rebase and run it again.

}

// RepoType contains possible types of repository
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@@ -232,6 +232,14 @@
"description": "Limit of result\n\nmaximum: setting.ExplorePagingNum",
"name": "limit",
"in": "query"
},
Copy link
Member

Choose a reason for hiding this comment

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

Do not add swagger stuff manually because it will be overwritten if you run make generate-swagger.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never touched this file manually. It was generated by make generate-swagger (exactly mingw32-make generate-swagger).

@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch 2 times, most recently from 2f0a8a2 to 2b16b01 Compare August 21, 2017 21:19
@Morlinest
Copy link
Member Author

@JonasFranzDEV Please comment first 2 commits in #2326. This PR is based on it (master -> #2326 -> #2343).

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 21, 2017
@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch 2 times, most recently from f1289e2 to 8cf8380 Compare September 16, 2017 14:01
@Morlinest Morlinest changed the title Custom search for each filter in repo-search component Use custom search for each filter type in dashboard Sep 16, 2017
@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch from 8cf8380 to 9a7a5a0 Compare September 16, 2017 14:41
@codecov-io
Copy link

codecov-io commented Sep 16, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2343   +/-   ##
=======================================
  Coverage   26.84%   26.84%           
=======================================
  Files          89       89           
  Lines       17608    17608           
=======================================
  Hits         4727     4727           
  Misses      12195    12195           
  Partials      686      686

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 266ebf8...27a4890. Read the comment docs.

@Morlinest
Copy link
Member Author

Rebased on top of #2371 + #2326. Only last commit is new.

@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch 3 times, most recently from c520dfc to 62655d8 Compare September 21, 2017 09:58
@Morlinest Morlinest closed this Sep 22, 2017
@lunny lunny removed this from the 1.x.x milestone Sep 24, 2017
@Morlinest Morlinest reopened this Oct 26, 2017
@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch 2 times, most recently from d96fd6a to dd7b4e5 Compare October 26, 2017 21:51
@Morlinest
Copy link
Member Author

PR reopened as all needed features were merged. Rebased on master, ready for testing, review and comments.

@lafriks
Copy link
Member

lafriks commented Oct 26, 2017

@JonasFranzDEV please review again

@Morlinest Morlinest force-pushed the feature-dashboard-filters-custom-search branch from c2d1538 to 86a07f8 Compare November 1, 2017 15:29
@Morlinest
Copy link
Member Author

@lafriks FYI he was actually reviewing another PR code (as this PR was not based on master). That code is not part of this PR anymore.

@lunny lunny added this to the 1.3.0 milestone Nov 1, 2017
@Morlinest
Copy link
Member Author

Morlinest commented Nov 1, 2017

Changed UI by removing second loader that was causing "flickering" effect if search was fast. Now the repository counter is reset on each filter change. Adding new screenshots (limit set to 5 repositories for this tests).

Edit: Screenshots moved to PR description.

@lunny
Copy link
Member

lunny commented Nov 1, 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 Nov 1, 2017
@lafriks
Copy link
Member

lafriks commented Nov 1, 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 Nov 1, 2017
@lafriks
Copy link
Member

lafriks commented Nov 1, 2017

Make LG-TM work

@lafriks lafriks merged commit 25acd6c into go-gitea:master Nov 1, 2017
@Morlinest Morlinest deleted the feature-dashboard-filters-custom-search branch November 1, 2017 19:39
@lafriks
Copy link
Member

lafriks commented Nov 1, 2017

@JonasFranzDEV I hope you had no requested changes anymore. Just after merging noticed that you have not approved yet

@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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants