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

replaced localStorage with settingsStore method #1185

Merged
merged 1 commit into from
Dec 29, 2023
Merged

replaced localStorage with settingsStore method #1185

merged 1 commit into from
Dec 29, 2023

Conversation

Greeshmanth1909
Copy link
Contributor

resolves #1171
As the title says, replaced localStorage method with settingsStore method.
Heres a list of tests I performed:

  • ran npm run serve and npm run preview and tested for chrome and firefox in both Jquery and serviceWorker modes.
  • couldn't try the app in IE mode as I do not have a windows machine
  • ran npm test all tests were successful
  • tested the extension in developer mode on chrome
  • tested the firefox extension

In all the tests, I was able to open the application and view .zim files. I also checked the localStorage and verified the presence of kiwixjs- prefix.

@Jaifroid
Copy link
Member

@Greeshmanth1909 Thanks for your PR. I'll review shortly.

@Greeshmanth1909
Copy link
Contributor Author

Greeshmanth1909 commented Dec 28, 2023

what went wrong and can I please know how to replicate this error?

@Jaifroid
Copy link
Member

Probably just a temporary error. Tests can be a bit unstable if there's a lot of load on the system. I'll re-run.

@Greeshmanth1909
Copy link
Contributor Author

Got it, thanks!

@Greeshmanth1909
Copy link
Contributor Author

@Jaifroid Are there any more tests?

@Jaifroid
Copy link
Member

@Jaifroid Are there any more tests?

No, just a few manual ones from me. I'll do those now.

@Jaifroid
Copy link
Member

Just to confirm that it works fine in IE11.

@Jaifroid
Copy link
Member

My other manual tests are all fine. I'll review code now.

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

The changes all look good, many thanks @Greeshmanth1909! Also many thanks for listing all the testing you've done, it really helps to know where I need to concentrate my own testing. Please ignore the one comment on the code, this is just a note to self about a possible separate issue unrelated to this PR.

Let me know when you're happy for me to squash/merge. You will appear as the PR author.

@@ -234,7 +235,7 @@ function loadPreviousZimFile () {
// It's a bit hacky but it works and I am not sure if there is any other way ATM
setTimeout(() => {
if (window.params.isFileSystemApiSupported || window.params.isWebkitDirApiSupported) {
const filenames = localStorage.getItem('zimFilenames');
const filenames = settingsStore.getItem('zimFilenames');
if (filenames) updateZimDropdownOptions(filenames.split('|'), '');
}
}, 200);
Copy link
Member

Choose a reason for hiding this comment

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

This is just a note to self, not part of this PR, but this timeout may well be the problem with inconsistency in loading the contents of archives. It introduces a race condition, and needs looking at as a separate issue.

@Greeshmanth1909
Copy link
Contributor Author

@Jaifroid Thats great! Please go ahead and merge the PR.
Thank you!

@Jaifroid Jaifroid merged commit fbc5af7 into kiwix:main Dec 29, 2023
6 checks passed
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.

Some file handling keys in local storage do not have the kiwixjs- prefix
2 participants