Skip to content

Conversation

harryxu
Copy link
Contributor

@harryxu harryxu commented Oct 1, 2017

Current user and repo search input is not support keyboard navigation and must use mouse click to select item.

So I use Semantic UI's Search component replace the jquery ajax code and html generating code, for improve the UI experience.

Here is the screenshots:

New user search

New repo search

Old user search (Can only use mouse select):

@codecov-io
Copy link

codecov-io commented Oct 1, 2017

Codecov Report

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

Impacted file tree graph

@@ Coverage Diff @@ ## master #2636 +/- ## ======================================= Coverage 27.11% 27.11% ======================================= Files 86 86 Lines 17062 17062 ======================================= Hits 4627 4627 Misses 11757 11757 Partials 678 678

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 a04718a...916ad17. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 1, 2017
$searchRepoBox.search({
minCharacters: 2,
apiSettings: {
url: suburl + '/api/v1/repos/search?q={query}',
Copy link
Member

Choose a reason for hiding this comment

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

uid query parameter is missing.

Copy link
Contributor Author

@harryxu harryxu Oct 2, 2017

Choose a reason for hiding this comment

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

@daviian Fixed.

Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

Looks good! Just one nit.

hideWhenLostFocus('#search-user-box .results', '#search-user-box');
}

// FIXME: merge common parts in two functions
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove this comment now that it is stale

@daviian
Copy link
Member

daviian commented Oct 2, 2017

Please remove comment as @ethantkoenig mentioned, otherwise 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 Oct 2, 2017
@harryxu
Copy link
Contributor Author

harryxu commented Oct 2, 2017

@ethantkoenig @daviian Done. 😃

@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Oct 2, 2017
@lunny lunny added this to the 1.3.0 milestone Oct 2, 2017
@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 2, 2017
@ethantkoenig
Copy link
Member

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 Oct 2, 2017
@lafriks
Copy link
Member

lafriks commented Oct 2, 2017

Make LG-TM work again

@lafriks lafriks merged commit b3cfa5a into go-gitea:master Oct 3, 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. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality

7 participants