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: Margin missing sometimes in the PR sidebar #4866

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Fix: Margin missing sometimes in the PR sidebar #4866

merged 1 commit into from
Oct 4, 2021

Conversation

cheap-glitch
Copy link
Member

@cheap-glitch cheap-glitch commented Oct 4, 2021

Fixes #4642

Test URLs

Steps to reproduce the bug:

  1. Go to repo home & reload the page
  2. Navigate to the PR tab
  3. Open a PR

Screenshot

capture-1633343624.mp4

@@ -9,6 +9,7 @@
}

/* Align the top of the sidebar with the main page content */
.rgh-clean-repo-sidebar .gutter-condensed > :last-child {
Copy link
Member Author

Choose a reason for hiding this comment

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

This selector was also affecting the sidebar in PR discussions

@cheap-glitch cheap-glitch marked this pull request as ready for review October 4, 2021 10:35
@fregante fregante added the bug label Oct 4, 2021
@fregante
Copy link
Member

fregante commented Oct 4, 2021

The change is reasonable, but the underlying issue probably still remains. Is the differing DOM GitHub’s fault or ours? If it's their fault then this change is 100% fine.

@fregante
Copy link
Member

fregante commented Oct 4, 2021

Checked: It's our fault. Try:

  1. Disable RGH
  2. Follow your screencast
  3. Enable the extension (in Firefox everything will run immediately on the current page without refresh)
  4. No bug

Then try:

  1. With RGH enabled
  2. Follow your screencast
  3. See bug

@cheap-glitch
Copy link
Member Author

The change is reasonable, but the underlying issue probably still remains.

The underlying "issue" is that the styles from clean-repo-sidebar stay loaded when navigating in ajax, so the affect a page they shouldn't (which is why the bug disappears upon refreshing the page).

@fregante
Copy link
Member

fregante commented Oct 4, 2021

Gotchaaaa, it's all because of the classrgh-clean-repo-sidebar.

You can just add it to this page and see the issue.

Let's fix that instead, CSS should not add to undo, it should only affect what it needs to affect and nothing more (ideally)

@fregante fregante merged commit bee79da into refined-github:main Oct 4, 2021
@fregante
Copy link
Member

fregante commented Oct 4, 2021

D'oh, that's exactly what you did 😂 🙌

@fregante
Copy link
Member

fregante commented Oct 4, 2021

Love to see many bugfixes in the upcoming release, which will happen this week.

@cheap-glitch cheap-glitch deleted the fix-missing-margin branch October 4, 2021 15:26
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.

Margin missing sometimes in the PR sidebar
2 participants