-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Permission Grid: stick the headers to handle a lot of tags #2887
Conversation
Hello! Thank you for the PR, I have actually played with the idea a bit in the past, and really liked it. We will review this soon when we pick up development again 🤗 |
Take your time, all of you deserve it! I'd be glad to start trying to improve a little Flarum with my limited skill but there is no rush at all, I'll wait for your review :) |
@Ornanovitch thanks so much, this is awesome. I've assigned a couple of reviewers in the hope to push this into the next release 🤞 . The ux on the permissions page is seriously hampered with many tags or groups, so thank you for thinking along! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! a few comments
Can you add to
.PermissionGrid-child {
td, th {
position: relative;
a z-index: 0
as it can overlap the th when hovered.
…e into or/sticky-PermissionGrid
This reverts commit 3eddb97.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, left a nit.
Is there a reason we haven't merged this yet, or have we just not gotten around to it yet?
@@ -49,19 +50,36 @@ | |||
color: @muted-color; | |||
} | |||
thead th { | |||
position: -webkit-sticky; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't suppose we can avoid vendor specific stuff here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now we need it for some old safari browsers
Just needed a second approval! |
The one I had suggested here: #2887 (review)
…3065) The one I had suggested here: #2887 (review)
Fixes #2810
Temporary fix before flarum/issue-archive#253
Changes proposed in this pull request:
Stick
thead th
andtbody th
: when you have a lot of tags permissions to handle, it's more consistent to be able to see the headers you're in.Reviewers should focus on:
To be tested with a lot of
Restrict by Tag
🏷️Does it work on safari?
Screenshot
FA isn't loaded on my local dev environment, I don't know why but it has nothing to do with this PR 😸
Confirmed