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

Fix position of sticky file headers after opening notification #5930

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

cheap-glitch
Copy link
Member

Fixes #5914

Test URLs

  1. Open a notification pointing to a PR (or PR files)
  2. Go to the Files tab
  3. This still happens if you go back to the "Conversation" tab, reload the page, and go to "Files" again

Screenshot

Before

01-before

After

02-after

Comment on lines +8 to 10
.notification-shelf ~ .application-main .file-header:not(.sticky-file-header) {
top: 129px;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently GitHub uses JS to compute and inject the correct value for top in each of the file header elements' style attribute 😬

This rule was superseded by those inline styles but still messed with their computations, resulting in a value of 198px instead of the correct 129px.

@fregante
Copy link
Member

Thank god

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.

I broke this probably a month ago 🥲

Would you be able to push a hotfix for 22.8.1 and 22.8.23? I think it looks like:

.notification-shelf ~ .application-main .file-header.sticky-file-header {
 	top: 198px;
}

@cheap-glitch
Copy link
Member Author

I think it looks like:

No, a bigger value for top will only make it worse 😄

Just tested the rule but with 60px (height of the sticky PR header) and it seems to work fine. Will push the hotfix asap.

cheap-glitch added a commit that referenced this pull request Aug 25, 2022
cheap-glitch added a commit that referenced this pull request Aug 25, 2022
cheap-glitch added a commit that referenced this pull request Aug 25, 2022
@cheap-glitch cheap-glitch merged commit 833a771 into main Aug 25, 2022
@cheap-glitch cheap-glitch deleted the fix-sticky-file-header branch August 25, 2022 11:39
@cheap-glitch
Copy link
Member Author

Hmm, weird I pushed some hotfixes but even after clearing the cache they don't get downloaded 🤔

@fregante Are you seeing them? Should be present for the last three versions

@fregante
Copy link
Member

fregante commented Aug 25, 2022

Are you seeing them?

Are you using the dev version? They're not applied to 0.0.0

@cheap-glitch
Copy link
Member Author

cheap-glitch commented Aug 25, 2022

Are you using the dev version? They're not applied to 0.0.0

I know, which is why I tried in my regular browser profile but no luck. And I know the CSS is valid because adding it to my custom CSS fixes things 🤷‍♂️

Might be an issue with the download/storage of the hotfixes?

Edit: Wait, did the hotfixes move to the new yolo repo? Because I added them to the hotfix branch 🤦‍♂️

Edit n°2: Added the hotfix to yolo but still don't see it 😢

@fregante
Copy link
Member

Wait, did the hotfixes move to the new yolo repo?

Yes, we'll drop the hotfixed branch soon.

Edit n°2: Added the hotfix to yolo but still don't see it 😢

I see it on Safari 22.8.1:

Screen Shot

Run anywhere to find out if the cache was downloaded (keep in mind that you need to open GitHub.com after clearing your cache)

chrome.storage.local.get(console.log)

@fregante
Copy link
Member

The hotfix works for me:

  • red: correct position
  • blue: hotfix, in style tag
  • purple: our previous style from refined-github.css

Screen Shot 1

@fregante
Copy link
Member

The only related bug I'm experiencing is #5857, because the its bug fix was in 22.8.8 but Safari is on 22.8.1 🥲

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.

Sticky file header in PRs is sometimes too low
2 participants