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

Added support for the kagi search engine. #21

Merged
merged 5 commits into from
Jul 11, 2023
Merged

Conversation

danbush
Copy link
Contributor

@danbush danbush commented Jul 9, 2023

Just a small update to support the privacy-focused search engine, Kagi!

@Fivefold
Copy link
Owner

Fivefold commented Jul 9, 2023

Hey @danbush, thanks a lot for taken the time to get to know the codebase and make the necessary changes! I really appreciate it. I checked your PR out a bit and it looks good so far but would need a few changes.

The major problem though is that kagi is behind a paywall and while there's a trial tier it's only 100 searches for one time. This is enough to test this PR but means I can't properly test or support this in the future if something breaks. The searches will eventually run out.

Are you willing to become a proper maintainer (for the kagi side of things)?

@danbush
Copy link
Contributor Author

danbush commented Jul 9, 2023

Hey! I've never been a maintainer for something beyond my personal projects (or at work), so I'm not 100% sure what I'd be signing up for, but if it would just mean updating it as needed, sure I'm willing! I made these changes mostly just for myself, so I'd be fixing it as bugs come up either way haha

Copy link
Owner

@Fivefold Fivefold left a comment

Choose a reason for hiding this comment

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

I reviewed your changes. Thanks again for the work so far.

scss/variables.scss Outdated Show resolved Hide resolved
scss/injectionBox.scss Outdated Show resolved Hide resolved
src/searchInjection.js Outdated Show resolved Hide resolved
src/searchInjection.js Show resolved Hide resolved
@Fivefold
Copy link
Owner

Fivefold commented Jul 9, 2023

Hey! I've never been a maintainer for something beyond my personal projects (or at work), so I'm not 100% sure what I'd be signing up for, but if it would just mean updating it as needed, sure I'm willing! I made these changes mostly just for myself, so I'd be fixing it as bugs come up either way haha

Yes, it's just updating when something breaks for Kagi and/or testing when I change something in the injection code that might affect Kagi.

@Fivefold
Copy link
Owner

Thanks for the additional commits. There were still a few small issues, I just took them on myself. I also made two comments in the review conversations above.

Automatic theme detection should work now, maybe you can give it a spin on your end just to make sure.

Unfortunately there's still a little bit of broken styling: when you explicitly set dark or light style in the extension settings and then switch between explicit dark or light theme in kagi the font color of bookmark descriptions change. There should be no changes in colours when an explicit theme is set for the injection box.

It's still usable and I couldn't figure out a way to make it work today, so I will leave it as is for now. If you are feeling motivated, you can try your hand. Otherwise I will merge this as is since I don't think anyone actually uses the explicit styles anyway.

@@ -165,9 +163,10 @@ div#bookmark-list-container {
&.dark,
.dark-bg &:not(.light), // DuckDuckGo
body[data-dt="1"] &:not(.light), // Google
:root:not(.light) &:not(.light), // Brave Search
Copy link
Owner

Choose a reason for hiding this comment

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

The brave search selector was actually too general and messing with the kagi ones. Without this automatic theme detection wasn't working properly for me. Just FYI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Makes sense! ANd yeah, the default theme was just something I missed in my original commit, but was in my system locally

@danbush
Copy link
Contributor Author

danbush commented Jul 10, 2023

Unfortunately there's still a little bit of broken styling: when you explicitly set dark or light style in the extension settings and then switch between explicit dark or light theme in kagi the font color of bookmark descriptions change. There should be no changes in colours when an explicit theme is set for the injection box.

Yeah I noticed this. I think its at least related to the fact that kagi has some styles specifically attached to OS-level dark mode that ignores the website-level decision. Kinda weird. IMO good enough to push as it, and if anyone other than me ends up using this and requests a fix I can look back at trying to solve it 😄

@Fivefold
Copy link
Owner

That's reasonable! Happy to merge, thank you again, will go live in the next release

@Fivefold Fivefold merged commit 691fde0 into Fivefold:master Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants