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

UI: Make repo and file titles sticky #274

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

prtksxna
Copy link

@prtksxna prtksxna commented Jan 4, 2018

screen shot 2018-01-04 at 9 22 38 am

@prtksxna
Copy link
Author

@legoktm, I don't think this is getting merged anytime soon.

@sahildua2305
Copy link
Contributor

@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.

Copy link
Contributor

@sahildua2305 sahildua2305 left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some issues with the z-index of the repo title:
hound

I'd propose adding the following here:

padding: 1%;
z-index: 2;

padding: 1% is to make the title look a little natural.

@@ -252,6 +255,8 @@ button:focus {
display: block;
line-height: 30px;
background-color: #f5f5f5;
position: sticky;
top: 37px;
Copy link
Contributor

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.

@sahildua2305
Copy link
Contributor

hound

This is how it looks like after the suggested changes, @prtksxna.

@kellegous what do you think about these changes?

@prtksxna
Copy link
Author

prtksxna commented Feb 5, 2018

@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.

@sahildua2305
Copy link
Contributor

@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?

Copy link
Contributor

@sahildua2305 sahildua2305 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 this, @prtksxna. LGTM.

@kellegous @stanistan can you please help in shipping this?

@legoktm
Copy link
Contributor

legoktm commented Feb 26, 2018

@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.

ollyja added a commit to ollyja/hound that referenced this pull request Oct 19, 2018
* UI: Make repo and file titles sticky
Base automatically changed from master to main February 24, 2021 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants