-
Notifications
You must be signed in to change notification settings - Fork 578
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
UI: Make repo and file titles sticky #274
base: main
Are you sure you want to change the base?
Conversation
prtksxna
commented
Jan 4, 2018
@legoktm, I don't think this is getting merged anytime soon. |
@prtksxna thanks a lot for your pull request. It took me some time to get to this PR. I'll review and test it in the upcoming week. Thanks for your patience. PS: I personally like the change as it's useful when a single repository has a large number of matches. |
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.
Thanks again, @prtksxna. I have made suggestions in comments. Let me know what you think of them.
@@ -224,6 +224,9 @@ button:focus { | |||
color: #666; | |||
font-size: 24px; | |||
padding-bottom: 5px; | |||
position: sticky; | |||
top: 0; | |||
background: #fff; |
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.
ui/assets/css/hound.css
Outdated
@@ -252,6 +255,8 @@ button:focus { | |||
display: block; | |||
line-height: 30px; | |||
background-color: #f5f5f5; | |||
position: sticky; | |||
top: 37px; |
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'd propose changing this to:
top: 5%;
z-index: 1;
in order to accommodate the changes with padding and z-index for repo title.
This is how it looks like after the suggested changes, @prtksxna. @kellegous what do you think about these changes? |
@sahildua2305 Hey! These changes look good to me. Do you want me to amend this commit and force push, or just push another commit and you'll squash when you merge? Also, sorry about the other comment. Now that I read it, it looks passive aggressive. I only posted it so that @legoktm could update our instance. |
@prtksxna Either way works well for me. I haven't seen us preferring one way over another here in this repository. Regarding your instance, I really like some of the features that you have - especially, the one with categorizing the repositories to enable search in one of the categories. How do you maintain it? I'd like to do the same for my company's internal instance. I'll be interested to see how we can bring those changes you have made within your organization to this project. Can you please share? |
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.
Thanks for this, @prtksxna. LGTM.
@kellegous @stanistan can you please help in shipping this?
@sahildua2305 Glad to hear you like our customizations! It's actually a bit hackily implemented, the repository filter didn't scale to our needs, so I set up multiple instances of hound, and wrote a web proxy to send requests to the specific backend people chose. There are some more details at https://www.mediawiki.org/wiki/Codesearch and the code is linked on that page as well. I'd love to make it more reusable and even integrate it into upstream hound if that's wanted. |
* UI: Make repo and file titles sticky