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

Add sticky-csv-header feature #6020

Merged
merged 9 commits into from
Oct 26, 2022

Conversation

FloEdelmann
Copy link
Member

@FloEdelmann FloEdelmann commented Sep 25, 2022

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.

Thanks for opening the PR! The feature is reasonable, CSS-only, short and you already wrote it, tested and documented it.

Hard to reject it for #5892 🤭

source/refined-github.ts Outdated Show resolved Hide resolved
source/refined-github.ts Show resolved Hide resolved
source/refined-github.ts Outdated Show resolved Hide resolved
Co-authored-by: Federico Brigante <me@fregante.com>
@@ -0,0 +1,33 @@
.markdown-body .csv-data {
Copy link
Member

Choose a reason for hiding this comment

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

Does this apply only to CSV data and other tabular data?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it does.

@@ -0,0 +1,33 @@
.markdown-body .csv-data {
max-height: 100vh;
Copy link
Member

Choose a reason for hiding this comment

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

On second thought… nested scroll areas aren't my favorite and this only works well if you perfectly align the scroll area to the viewport. It shouldn't be necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

It is necessary, since that element is already scrolling horizontally.

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 think the nested scroll-area makes this unusable :(

Achieving your screenshot means carefully scrolling the main document to fit the table.

Overscrolling means that the headers are sticky, but outside the viewport, so still not visible; underscrolling means that the headers are sticky, but in the middle of the viewport. Fixing the main scroll means having to find a corner of the viewport outside this nested scroll area.

And I don't even wanna think of how impossible this is on mobile.

We can only merge it if there's a way to have the sticky headers without using max-height: 100vh

@FloEdelmann
Copy link
Member Author

FloEdelmann commented Oct 2, 2022

I've committed an alternative solution now that lets the whole page scroll instead of only the table. However, the other elements also scroll away horizontally now. We might be able to fix that though by applying position: sticky; left: 0; to all of them.

grafik

@fregante
Copy link
Member

fregante commented Oct 2, 2022

Nice trick, but this looks broken

Screen Shot

@fregante
Copy link
Member

fregante commented Oct 8, 2022

How about only making the row headers sticky? That would be an improvement already, even if not complete

@FloEdelmann
Copy link
Member Author

I've changed it to make only the row headers sticky.

/* Restore zebra-colored rows in CSV files */
.blob-wrapper.type-csv table tr:nth-child(2n) {
background-color: var(--color-canvas-subtle);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing this in the latest version, so I'll drop it (also because unrelated). Feel free to open a new PR

Screen Shot 4

@fregante fregante changed the title Add sticky-csv-header CSS-only feature Add `sticky-csv-header feature Oct 26, 2022
@fregante fregante merged commit 050dff5 into refined-github:main Oct 26, 2022
@fregante fregante changed the title Add `sticky-csv-header feature Add sticky-csv-header feature Oct 26, 2022
@FloEdelmann FloEdelmann deleted the sticky-csv-header branch October 26, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants