Skip to content

Notifications - Step 2 #539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 5, 2017
Merged

Notifications - Step 2 #539

merged 5 commits into from
Jan 5, 2017

Conversation

andreynering
Copy link
Contributor

UI for notifications

@andreynering andreynering added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Dec 30, 2016
@andreynering andreynering added this to the 1.1.0 milestone Dec 30, 2016
bkcsoft
bkcsoft suggested changes Dec 30, 2016
View reviewed changes
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 💯 🎉

cmd/web.go
@@ -168,6 +168,8 @@ func runWeb(ctx *cli.Context) error {

bindIgnErr := binding.BindIgnErr

m.Use(user.GetNotificationCount)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this add unnecessary overhead for endpoints like /api that will never use it? :)

Copy link
Member

@lunny lunny Dec 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

^, Only UI route group need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public/less/_user.less
margin-left: 7px;
}

.octicon-issue-opened, .octicon-git-pull-request {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these defined somewhere already that we can reuse? (for the activity/dashboard feed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

@metalmatze
Copy link
Contributor

If you're creating UI changes a little screenshot would be pretty helpful for just browsing github! 👍 😊

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 2, 2017
@andreynering
Copy link
Contributor Author

Here a few pics:

It's working, but still very simple and missing some key functionality: pagination (must have), mark as read/unread, pinning a notification, others. Should this be done in this PR or on another ones?

@bkcsoft
Copy link
Member

bkcsoft commented Jan 2, 2017 •
edited
Loading

Pagination is a must for this one, the others can be extended on in a later PR if necessary :)

@lunny
Copy link
Member

lunny commented Jan 3, 2017

^

@andreynering andreynering removed the pr/wip This PR is not ready for review label Jan 3, 2017
@lunny
Copy link
Member

lunny commented Jan 5, 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 Jan 5, 2017
@appleboy
Copy link
Member

appleboy commented Jan 5, 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 Jan 5, 2017
@strk
Copy link
Member

strk commented Jan 5, 2017

@bkcsoft are your requested changes good now ?

@andreynering andreynering merged commit 79d5271 into go-gitea:master Jan 5, 2017
@gsantner
Copy link

gsantner commented Jan 5, 2017 •
edited
Loading

@andreynering
The Link in the UI is wrong when behind reverse proxy.
Mine is under https://mydomain.xyz/gogitea and everything links corretctly to subpaths (e.g. https://mydomain.xyz/gogitea/issues) but this one links to https://mydomain.xyz/notifications which, apparently doesn't exist.

@andreynering
Copy link
Contributor Author

@gsantner Thank you for pointing. That's easy to fix.

@strk strk mentioned this pull request Jan 5, 2017
Closed
10 tasks
@andreynering andreynering deleted the notifications-step-2 branch February 4, 2017 12:45
@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.
Reviewers

@lunny lunny lunny left review comments

@bkcsoft bkcsoft bkcsoft requested changes

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
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
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
8 participants
@andreynering @metalmatze @bkcsoft @lunny @appleboy @strk @gsantner @tboerger