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

Issue/PR Context Popups #9822

Merged
merged 24 commits into from
Jan 20, 2020
Merged

Issue/PR Context Popups #9822

merged 24 commits into from
Jan 20, 2020

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Jan 17, 2020

Context popup when hovering over linked issue/PRs.

There are a few things missing because they weren't easily available, but overall I think this is enough to warrant a PR.

Action Light Dark
Open Issue Screenshot from 2020-01-16 22-56-49 dark1
Closed Issue Screenshot from 2020-01-16 22-56-51 dark2
Open PR Screenshot from 2020-01-16 22-56-55 dark3
Closed PR Screenshot from 2020-01-16 22-56-58 dark4
Merged PR Screenshot from 2020-01-16 22-57-00 dark5
Bonus! Cross-Reference light-bonus dark-bonus

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 17, 2020
@lunny lunny added this to the 1.12.0 milestone Jan 17, 2020
@lunny lunny added the type/changelog Adds the changelog for a new Gitea version label Jan 17, 2020
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

This looks great. If this could be used for the PR list and show information like number of approvals, blocking issues or mergeability, we could tick a couple of boxes in #8659.

I'm approving nonetheless because that can be done in a 2nd PR.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 17, 2020
@GiteaBot GiteaBot 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 17, 2020
Copy link
Contributor

@bagasme bagasme left a comment

Choose a reason for hiding this comment

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

I think text inside the popup is almost unreadable on dark mode.

@silverwind
Copy link
Member

Dark style needs some fixes. A less intense border and the triangle pointer. I'd suggest not styling this particular popup, but .ui.popup in dark style so it applies to all of them.

Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser
Copy link
Member Author

jolheiser commented Jan 17, 2020

@silverwind I updated the OP screenshots, do they look better now?

@bagasme I tried to make the color lighter, but imho it looks better as-is. This is a little too bright.

For reference, the original color is the same as #1 Test in the screenshot. The "lighter text" is the same color as user2. It looks better to highlight something, but when it's used for all the text it looks too bright imo

Lighter Text
lighter_text

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@lunny
Copy link
Member

lunny commented Jan 17, 2020

If you can move the related js codes on a spereate js file, that will be perfect!!!

@silverwind
Copy link
Member

silverwind commented Jan 17, 2020

@jolheiser much better, thanks. I'd maybe go a shade darker even on the border, but no biggie.

@lunny good idea. I'd suggest moving the code to web_src/js/features/contextPopups.js and then import it on index.js. It could look similar to this:

export function initContextPopups() {
  const refIssues = $('.ref-issue');
  if (!refIssues.length) return;

  // your code here
}

and load via

import { initContextPopups } from './features/contextPopups.js';

web_src/js/index.js Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

nice PS: for js stuff i point to silverwind ....

@codecov-io
Copy link

codecov-io commented Jan 17, 2020

Codecov Report

Merging #9822 into master will decrease coverage by 0.03%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9822      +/-   ##
==========================================
- Coverage   42.27%   42.24%   -0.04%     
==========================================
  Files         605      605              
  Lines       79251    79262      +11     
==========================================
- Hits        33505    33484      -21     
- Misses      41616    41644      +28     
- Partials     4130     4134       +4
Impacted Files Coverage Δ
modules/auth/auth_form.go 100% <ø> (ø) ⬆️
modules/auth/ldap/ldap.go 54.4% <ø> (ø) ⬆️
models/user.go 50.16% <0%> (-0.3%) ⬇️
cmd/admin_auth_ldap.go 79.32% <0%> (-1.36%) ⬇️
routers/admin/auths.go 30.95% <100%> (+0.23%) ⬆️
services/pull/patch.go 62.89% <0%> (-6.92%) ⬇️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
models/unit.go 37.03% <0%> (-2.47%) ⬇️
routers/repo/view.go 38.59% <0%> (-0.88%) ⬇️
models/repo.go 47.02% <0%> (-0.13%) ⬇️

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 f605e23...c8c7b3d. Read the comment docs.

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser
Copy link
Member Author

@silverwind Done. Had to do it slightly different to avoid being yelled at by the linter.

@techknowlogick techknowlogick merged commit 7d7ab1e into go-gitea:master Jan 20, 2020
@jolheiser jolheiser deleted the popup_issue branch January 20, 2020 04:44
@mooror
Copy link

mooror commented Aug 3, 2020

Just came across this new feature in V12. Really polishes up the interface. Great work @jolheiser (and company ;) )

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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.

None yet