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

Restore hidden-review-comments-indicator #5950

Merged
merged 12 commits into from
Sep 9, 2022

Conversation

chcamiloam
Copy link
Contributor

@chcamiloam chcamiloam commented Sep 8, 2022

Test URLs

https://github.com/refined-github/sandbox/pull/18/files

Screenshot

Screen Shot 2022-09-08 at 11 07 41

Description

Class .blob-num was overwriting plugin's css, added !important to fix the issue.
Counter was not working, it was using the wrong selector

source/features/hidden-review-comments-indicator.css Outdated Show resolved Hide resolved
@@ -30,7 +30,7 @@

.rgh-comments-indicator button {
position: absolute;
right: 100%;
right: 90%;
Copy link
Member

@fregante fregante Sep 8, 2022

Choose a reason for hiding this comment

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

Why? Does it work with double-digit comments?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's ok

Copy link
Contributor Author

@chcamiloam chcamiloam Sep 8, 2022

Choose a reason for hiding this comment

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

The Comments icon too close to the screen border in 1800px with 100% imo, but let me change it :)

source/features/hidden-review-comments-indicator.css Outdated Show resolved Hide resolved
@@ -8,7 +8,7 @@

.rgh-comments-indicator:is(::before, ::after) {
content: '' !important; /* Needs to override .blob-num’s */
position: absolute;
position: absolute !important; /* Needs to override .blob-num’s */
Copy link
Member

@fregante fregante Sep 8, 2022

Choose a reason for hiding this comment

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

Is this selector doing anything? I don't see the line added by border-top in the your screenshot.

See:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class is working, but I'm not so sure about the selector, let me do some testing :)

@fregante fregante changed the title FIx hidden-review-comments-indicator is broken FIx hidden-review-comments-indicator is broken Sep 8, 2022
@fregante fregante changed the title FIx hidden-review-comments-indicator is broken Restore hidden-review-comments-indicator Sep 8, 2022
@fregante fregante added the bug label Sep 8, 2022
@chcamiloam
Copy link
Contributor Author

chcamiloam commented Sep 8, 2022

Removed the css, I took one by one the lines and it doesn't affect anything (the absolute is still the fix of the main issue).
I tried with right:100% and this is the result
Screen Shot 2022-09-08 at 13 11 44

I changed it to 95% this time, it should allow to not have issues with 2digits numbers

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

What I meant is that the line between 22 and 23 here is missing, you can see it in the gif I linked:

Screen Shot 2

border-top is supposed to draw that line, but that part is broken. That's why "it's not affecting anything"

@chcamiloam
Copy link
Contributor Author

Changed the structure to handle the icon with less css. Everything looks the same but the markup structure is different, add the line again.
Screen Shot 2022-09-08 at 14 46 25

@fregante
Copy link
Member

fregante commented Sep 8, 2022

This happens in split view:

Screen Shot 4

@chcamiloam
Copy link
Contributor Author

chcamiloam commented Sep 9, 2022

Added new selector to change markup in split-view, this may seem unnecessary but I really wanted the line in both number columns.
The space in the left is lower in split view, so it's not possible to have the number.

image

@fregante
Copy link
Member

fregante commented Sep 9, 2022

We try to keep the amount of code to a minimum. The previous "complex CSS" worked without changes to the DOM. I'd prefer to keep it that way

@chcamiloam
Copy link
Contributor Author

I can't see how to draw the line in both number columns without changing the markup, can I get a hand here? this is how it looks right now.

image

@@ -1,4 +1,4 @@
.rgh-comments-indicator {
:root .rgh-comments-indicator {
Copy link
Member

@fregante fregante Sep 9, 2022

Choose a reason for hiding this comment

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

This was the only fix required to make the button visible again.

The reason why the line isn't being shown is because GitHub now uses ::before to show the actual line, so our code fighting with GitHub’s own style.

An easier solution would be to either:

  • only use ::after, which doesn't seem to be used by GitHub
  • ensure that all the CSS actually applies OR figure out why it doesn't

I undid the previous changes because I don’t think they were going in the right direction for the most part. I'd be ok merging this as is for now and the line can be restored later.

Rather, this single line is good enough for now and if you can't fix the rest autonomously I'd have to fix it myself, which defeats the purpose of PRs

@fregante fregante merged commit 653552a into refined-github:main Sep 9, 2022
@@ -29,7 +22,7 @@

.rgh-comments-indicator button {
position: absolute;
right: 95%;
left: -40px;
Copy link
Member

@fregante fregante Sep 9, 2022

Choose a reason for hiding this comment

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

left: -40px only works for the specific icon size, font size and character count.
right: 100% is the correct alignment and it works with any amount of text in this button.

The only problem lies in the button’s content itself, which seems to have excessive padding (which exists to expand its clickable area, but it should be counteracted by negative margins)

Also there's an additional issue with the available space overall for this piece of UI. For example if there's the new file drawer open, perhaps we cannot show a comment count at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants