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.

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

Add clean-notifications feature #3051

Merged
merged 5 commits into from
May 3, 2020
Merged

Add clean-notifications feature #3051

merged 5 commits into from
May 3, 2020

Conversation

notlmn
Copy link
Contributor

@notlmn notlmn commented May 2, 2020

Closes #3030.

Test

https://github.com/notifications (only on inbox with group by repositories applied)

Screenshot

https://user-images.githubusercontent.com/37769974/80862883-0419bc80-8c96-11ea-84a7-15baf1eac0e6.png

@notlmn notlmn requested a review from fregante May 2, 2020 11:38
@notlmn
Copy link
Contributor Author

notlmn commented May 2, 2020

From #3030 (comment).

  1. Have't included .js-notifications-group as the dropdown check fails anyway expect only on "Inbox".
  2. I tried doing this, but that messed up all the UI, maybe because the elements are under different parents, or having a stray #text element. So ignored that as font difference for id (12px) and title (14px) is only 2px (1px on top and bottom).

fregante
fregante requested changes May 2, 2020
View reviewed changes
source/features/clean-notifications.tsx Outdated Show resolved Hide resolved
notlmn
notlmn commented May 2, 2020
View reviewed changes
source/refined-github.css Outdated
.js-notifications-group [id^='notification'] > .d-flex > .f6 > span {
display: block;
font-size: 12px;
font-variant-numeric: tabular-nums;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to be careful about some fonts with non-tabular numbers.

@fregante WDYT? Leave this here or remove it?

fregante
fregante reviewed May 2, 2020
View reviewed changes
source/refined-github.css Outdated Show resolved Hide resolved
@fregante fregante merged commit 3b182a0 into master May 3, 2020
@fregante fregante deleted the clean-notifications branch May 3, 2020 15:19
@FloEdelmann
Copy link
Member

@fregante @notlmn Now, the styles are restricted to a rgh-clean-notifications class which is not added anywhere (because there is no tsx feature file). I guess it was removed by accident in c272ba1.

notlmn added a commit that referenced this pull request May 3, 2020
@notlmn
Fix incorrect CSS selector for clean-notifications
09919eb 
CSS copied into file from on af3b9e1 in #3051 is not the one with the right CSS selector, which should be `.js-notifications-group`.
@notlmn
Copy link
Contributor Author

notlmn commented May 3, 2020

@fregante might've copied the CSS from the wrong commit.

Fixed in 09919eb.

@fregante
Copy link
Member

fregante commented May 3, 2020

D'oh! Thankfully this doesn't break anything but it means it will be published in the next release.

@fregante fregante mentioned this pull request Jun 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fregante fregante fregante requested changes

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make new notifications behave more like old notifications UI
3 participants
@notlmn @FloEdelmann @fregante