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 a toggle for sticky header #126

Merged
merged 9 commits into from
Jul 6, 2024
Merged

add a toggle for sticky header #126

merged 9 commits into from
Jul 6, 2024

Conversation

jason-ojisan
Copy link

09e12f8#r143854685
@JayXT does it look good?

@jason-ojisan jason-ojisan requested a review from a team as a code owner July 5, 2024 20:02
@JayXT
Copy link

JayXT commented Jul 5, 2024

@jason-ojisan, I'm not an expert in the Yomitan Rikaitan codebase, but overall the changes in this PR seem logical for me. Although I've noticed that 5 PR checks are failing, so perhaps they have to be addressed.
@tatsumoto-ren, what are your thoughts regarding the proposed changes?

@jason-ojisan
Copy link
Author

@JayXT just a side note: We stand against the hijack attempt of the (former) Yomichan project by moeway because they promote a flawed and ineffective language learning method which is different from AJATT. Don't say Yomitan. Say "Rikaitan" if you also believe the hijack should not succeed.

@tatsumoto-ren
Copy link
Member

what are your thoughts regarding the proposed changes?

I think it would be nice to figure out how to update the search page automatically when the toggle is flipped. But it's not a big deal.

@jason-ojisan
Copy link
Author

@JayXT try running the patched extension to see if that's what you expect.

@jason-ojisan
Copy link
Author

Although I've noticed that 5 PR checks are failing, so perhaps they have to be addressed.

The patch should be fine but the tests have to be fixed to work with the updated code. This extension is extremely shitty because it was written by idiots who don't know how to code. And JS/TS is a very bad language by itself. In a perfect world JS would not exist, and everything would be written in C++. But this is not a perfect world.

@JayXT
Copy link

JayXT commented Jul 6, 2024

@JayXT just a side note: We stand against the hijack attempt of the (former) Yomichan project by moeway because they promote a flawed and ineffective language learning method which is different from AJATT. Don't say Yomitan. Say "Rikaitan" if you also believe the hijack should not succeed.

Sorry for the confusion. Yes, I meant Rikaitan, or the original Yomichan. To my understanding the difference in codebase between the two hasn't gotten too wide yet.

By the hijack do you mean the name similarity, or there was some other controversial incident?

@JayXT
Copy link

JayXT commented Jul 6, 2024

@JayXT try running the patched extension to see if that's what you expect.

Are there any instructions how this can be done?

@tatsumoto-ren
Copy link
Member

Are there any instructions how this can be done?

Run build.sh. Then import the file named rikaitan-firefox-dev.xpi from the builds folder.

@tatsumoto-ren tatsumoto-ren merged commit cfb1ca0 into Ajatt-Tools:main Jul 6, 2024
6 of 8 checks passed
@jason-ojisan jason-ojisan deleted the sticky-header branch July 6, 2024 23:47
@JayXT
Copy link

JayXT commented Jul 7, 2024

FYI, I was able to build and generate rikaitan-firefox-dev.xpi on a VM after solving a couple of issues with missing npm and packages, but importing a custom extension into Firefox is quite an ordeal nowadays:) Changing xpinstall.signatures.required to false is no longer sufficient. Due to time constraints I might look into this later, or since the changes are already merged into main, waiting until the next Rikaitan release is another valid method.

@tatsumoto-ren
Copy link
Member

@JayXT Running a VM is not necessary at all. To import unsigned extensions you need this version of firefox: firefox-developer-edition.

@JayXT
Copy link

JayXT commented Jul 7, 2024

@tatsumoto-ren, yes, you're right using a VM is optional, but I still prefer to keep my workstation clean and stable always ready for immersion and sentence mining, so for such experiments usually I rely on one of VMs.

In any case, I was able to install the extension and verify the behavior of the toggle. It seems to be working fine, i.e. when sticky header is turned off, the header gets hidden on scrolling and overflow of header contents is set to visible, which as requested prevents unnecessary scrolling by showing the entire query text:

image

image

The only remaining question is that the following CSS is ignored by the header (the upper part has white background), but I guess that will be solved in some different PR:

body {
    background-color: #fffaf0;
    color: #2a1b0a;
}

As a workaround in the current Rikaitan stable version I've modified custom CSS to the following in order to achieve consistent appearance:

body, #content-scroll > * {
    background-color: #fffaf0;
    color: #2a1b0a;
}

@tatsumoto-ren
Copy link
Member

Try

:root {
    --background-color: #fffaf0;
    --text-color: #2a1b0a;
}

The extension uses variables a lot in the CSS so you can just override them instead of overriding the rules.

@JayXT
Copy link

JayXT commented Jul 7, 2024

That worked out. Thank you!

@JayXT
Copy link

JayXT commented Jul 9, 2024

FYI, @jason-ojisan, it looks like there was a new release and Rikaitan was updated, but for some reason the sticky header option introduced in this PR is not available. Have the changes been overwritten?

@tatsumoto-ren
Copy link
Member

tatsumoto-ren commented Jul 9, 2024

I have uploaded 24.7.1.0 to the extension stores. This option was added after 24.7.1.0, in 24.7.7.0.

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.

3 participants