kiwix / kiwix-js

Fully portable & lightweight ZIM reader in Javascript
https://www.kiwix.org/
GNU General Public License v3.0
310 stars 135 forks source link

Closes #127: Reopening last article via double clicking on menu items/refreshing when double clicking on home #1284

Closed D3V-D closed 1 week ago

D3V-D commented 2 weeks ago

Closes #127

Works in npm run serve and passes npm run test. Also works in both ServiceWorker and restricted modes.

Works fine in Firefox & Chromium for me, I do not have edge to test that so some help there would be much appreciated!

Also, if you could check npm run preview for me as the changes are not reflected for me, that'd be great.

Should be completely keyboard compatible

D3V-D commented 2 weeks ago

@Jaifroid Please review when you have the time, thanks!

Jaifroid commented 2 weeks ago

Great, thanks for the PR! I'm currently at work, but will look at this as soon as I can.

Jaifroid commented 2 weeks ago

I got a few minutes to test your implementation with npm run preview, and I'm happy to report it builds and opens fine in that view (for me), and is working really well. 😀 No problems in Edge. Congratulations!

You can also build the minified distribution and open it in http-server like this:

npm run build-min
npm run web-server

Then go to http://localhost:8080/dist/www/index.html in your browser. If you need to disable App Cache in that view, you can also add ?appCache=false to the end of the URL in your browser. This all works fine for me.

If you need to refresh the app in your browser, then make sure App Cache is disabled, open dev tools, make sure browser caching is turned off in dev tools, and then hard-refresh with Ctrl-Shift-R (or Mac equivalent). I have noticed that on Firefox even this sometimes fails to refresh code, and the only solution then is to exit the browser and then re-load the page. On Chromium browsers I've found this procedure always loads the latest code. The key is to refresh with dev tools open in the browser, and browser caching disabled in dev tools.

It'll take a bit more time to review the code itself, but I'll do it as soon as I can.

D3V-D commented 2 weeks ago

No rush, thanks for taking the time to test the preview.

Jaifroid commented 1 week ago

This one is a small bug. It concerns what happens when the UI is collapsed on narrow screens (you can use the device emulation tab in Dev Tools, at least in Chromium browsers to check this, or you can simply make the browser window very narrow). The issue you see here is that the code that collapses the menu again isn't executed in the return code, I suppose.

https://github.com/user-attachments/assets/bdeb7437-7523-4c4f-af1f-54afb28727f7

Jaifroid commented 1 week ago

There is a slight cosmetic inconsistency when using the keyboard (tab key) to select UI elements, which is that the focused button that has your styles applied no longer has the "halo" round the button. I suspect this is to do with focusing the button element vs focusing the li element or something like that (just guessing). It's worth saying that the halo effect doesn't in fact exist in the current release of the app, so maybe you added it. It looks a bit inconsistent here. The video here was made entirely using Tab and Shift-Tab to shift amongst the buttons at the top (after clicking on the top bar). There is very little visual clue that an already highlighted button is being focused by the keyboard, unlike the un-highlighted buttons.

https://github.com/user-attachments/assets/0dbb90f9-1211-41e0-b803-db6b9c429de8

D3V-D commented 1 week ago

This one is a small bug. It concerns what happens when the UI is collapsed on narrow screens (you can use the device emulation tab in Dev Tools, at least in Chromium browsers to check this, or you can simply make the browser window very narrow). The issue you see here is that the code that collapses the menu again isn't executed in the return code, I suppose.

Okay, so you're saying that it should auto-collapse when switching to a different page, gotcha. I'll work on that.

There is a slight cosmetic inconsistency when using the keyboard (tab key) to select UI elements, which is that the focused button that has your styles applied no longer has the "halo" round the button. I suspect this is to do with focusing the button element vs focusing the li element or something like that (just guessing). It's worth saying that the halo effect doesn't in fact exist in the current release of the app, so maybe you added it. It looks a bit inconsistent here. The video here was made entirely using Tab and Shift-Tab to shift amongst the buttons at the top (after clicking on the top bar). There is very little visual clue that an already highlighted button is being focused by the keyboard, unlike the un-highlighted buttons.

Good point; I'll try to make that focus halo have higher priority.

Will ping you once I've gotten these issues resolved.

D3V-D commented 1 week ago

@Jaifroid Fixed the two bugs! I think the menu auto-closing should work fine, but I would check on IE if the focus priority works

D3V-D commented 1 week ago

@Jaifroid Not sure if you're busy, but do let me know if you see any more bugs or anything with the PR!

Jaifroid commented 1 week ago

Thanks @D3V-D, I confirm those bugs are fixed in testing on Edge. I'll test on IE11 and an old Firefox shortly.

D3V-D commented 1 week ago

@Jaifroid sounds good to me - it just feels almost too easy 🤔

I'll probably do some more tests later today and make sure I'm happy with how it looks visually and everything them I'll let you know

D3V-D commented 1 week ago

@Jaifroid okay I haven't really found the time to do additonal testing but it should be fine functionally, any issues would just be aesthetic and slight.

I'm happy to merge now! Thanks for the review