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.

Sign up for GitHub

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

Jump to bottom

Hide article list on prefix blur or on Esc #499

Merged
merged 2 commits into from
May 19, 2019
Merged

Hide article list on prefix blur or on Esc #499

merged 2 commits into from
May 19, 2019

Conversation

Jaifroid
Copy link
Member

@Jaifroid Jaifroid commented May 16, 2019 •
edited
Loading

This fixes #39 and the regression in #442.

The behaviour is as follows:

  • User can dismiss search results with Esc, click/tap anywhere outside of prefix (search box), or with tab;
  • Search results are not lost, they are hidden;
  • If user re-enters prefix (if it gains focus), then the search results are shown again if prefix is not empty.

In the process, testing on FFOS where the on-screen-keyboard causes drastic screen resizes, I discovered an obsolete value still being used, erroneously in the resizeIframe() function. Since we now float articleListWithHeader over the article, we shouldn't subtract its height from the window.outerHeigh. It causes a drastically shortened screen otherwise.

I have coded this with jQuery for the sake of consistency with surrounding code.

Tested in Edge 44, Edge 75, FFOS.

@Jaifroid Jaifroid added the bug label May 16, 2019
@Jaifroid Jaifroid added this to the v2.6 milestone May 16, 2019
@Jaifroid Jaifroid requested a review from mossroy May 16, 2019 16:52
@Jaifroid Jaifroid self-assigned this May 16, 2019
@Jaifroid
Copy link
Member Author

Hmm, Travis seems to have fallen over on Edge, but not on any other browser. I have tested this extensively on Edge 44, and the code only affects the UI, not the content in any way. I wonder if this is a transitory issue.

mossroy
mossroy approved these changes May 18, 2019
View reviewed changes
Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

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

I quickly tested, and it seems to work well.
It would be great if it solves #39 : it's a very old issue.
If you're in the mood of implementing old issues, there is #55 in the same area ;-)

www/js/app.js Outdated Show resolved Hide resolved
@Jaifroid
Copy link
Member Author

If you're in the mood of implementing old issues, there is #55 in the same area ;-)

You're telepathic! I just implemented that today and was waiting to get this PR approved / merged before posting the code as a new PR.

@mossroy
Copy link
Contributor

mossroy commented May 18, 2019

Regarding the UI tests that fail on Edge 40 and 42, I tried to restart Travis several times, but they still fail.
We'll need to fix that before merging. I'm trying to find out what happens

@mossroy
Copy link
Contributor

mossroy commented May 18, 2019

Well, the issue on Edge UI tests is not obvious.
As we're talking about old versions of Edge, that you manually tested, it might not be worth investing too much time on that.
I already had to skip some UI tests on some versions of Edge : maybe we'll have to skip more.

@Jaifroid
Copy link
Member Author

Tests still failing on Edge 40. Edge 40 is pretty much only still used on Windows Mobile, and that platform is no longer being developed (though still supported with bugfixes for a while longer). I test manually for Windows Mobile when I port things to Kiwix JS Windows. So far I haven't found incompatibilities, but haven't tested this latest code there yet. If there were an incompatibility, I'd work around it there.

I'd say it's pretty safe to drop tests for Edge 40.

@Jaifroid Jaifroid mentioned this pull request May 18, 2019
Merged
@mossroy
Copy link
Contributor

mossroy commented May 19, 2019

OK, so I'll remove the UI tests for Edge 40 (but keep the unit tests on it).
I suggest to completely remove Edge 42 from the browser list, but to keep :

  • Edge 44 (as it is the last version based on EdgeHTML)
  • lastest Edge (for the upcoming version based on Chromium)

@Jaifroid
Copy link
Member Author

@mossroy I've compiled a test version of Kiwix JS Windows that uses this code for Windows Mobile (Edge 40), and have successfully run it there. So yes, it's safe to remove old versions. I have no idea why the tests are failing...

@mossroy
Copy link
Contributor

mossroy commented May 19, 2019

I've kept the beginning of UI tests on Edge (all versions), because they never failed (as far as I know) : opening the ZIM file and searching for articles are still covered. It's still better than no UI test at all.

I had to disable latest Edge, because I did not find an easy way to know its version, which is needed to distinguish Edge 44 (currently latest, which needs to disable some tests), and EdgeChromium (future latest, on which we should try to run all tests).

When EdgeChromium will be released, and available on SauceLabs, I'll re-enable latest Edge

@Jaifroid
Copy link
Member Author

OK, sounds good! Shall I go ahead and squash/merge this?

@Jaifroid
Copy link
Member Author

And if I merge, should I keep two commits, one for this issue and one for the unit tests?

@mossroy
Copy link
Contributor

mossroy commented May 19, 2019

Yes, you can go ahead.
It would be perfect to have 2 commits, if you manage to do it. But it's not compulsory...

@mossroy mossroy mentioned this pull request May 19, 2019
Closed
@Jaifroid Jaifroid force-pushed the Hide-article-list-on-prefix-blur-or-on-Esc branch from 02b87e0 to 4c79abe Compare May 19, 2019 11:40
@Jaifroid Jaifroid merged commit b5b2c64 into master May 19, 2019
@Jaifroid Jaifroid deleted the Hide-article-list-on-prefix-blur-or-on-Esc branch May 19, 2019 12:07
Krinkle added a commit to Krinkle/kiwix-js that referenced this pull request Nov 28, 2020
@Krinkle
Re-enable unit tests in latest Edge
563a6f0 
The unit tests were implicitly disabled after
kiwix#499 due to an issue with
the UI tests found on an older Edge version that was the default
"edge" version on SauceLabs in May 2019 (not sure which version
that was, the last pre-Chromium Edge version was 44, which was
passing, so perhaps SauceLabs defaulted to a beta release or
a much older version like 15-18?)

Now that Edge uses Chromium, try re-enabling the unit tests there.

Fixes kiwix#502.
Krinkle added a commit that referenced this pull request Nov 28, 2020
@Krinkle
Re-enable unit and UI tests in latest Edge
522a7db 
The tests were disabled after #499
due to an issue with the Edge version that was the default "edge" on
SauceLabs in May 2019 (not sure which version that was, the last pre-Chromium
Edge version was 44, which was passing, so perhaps SauceLabs defaulted
to a beta release, or used a much older version like 15-18?)

Now that Edge uses Chromium, try re-enabling the tests.

Fixes #502.
Krinkle added a commit that referenced this pull request Nov 28, 2020
@Krinkle
Re-enable unit and UI tests in latest Edge
590bcfc 
The tests were disabled after #499
due to an issue with the Edge version that was the default "edge" on
SauceLabs in May 2019 (not sure which version that was, the last pre-Chromium
Edge version was 44, which was passing, so perhaps SauceLabs defaulted
to a beta release, or used a much older version like 15-18?)

Now that Edge uses Chromium, try re-enabling the tests.

Fixes #502.
mossroy pushed a commit that referenced this pull request Jan 3, 2021
@Krinkle
update QUnit, use Karma, make some of CI work locally as well (#672)
8b3a5c3 
* workflows: Rename "CI" to "Release"

Rename this to make space for a separate "CI" workflow that will
run continuous integration and testing jobs for commits and PRs.

* workflows: add CI workflow for unit tests and support running locally

* Load qunit package from npm, this is the start of a larger transition.
  ref at #554.

* Update QUnit from 2.3 to 2.13.

* Use Karma for the running of unit tests (instead of Nightwatch).

  While it was possible to use a fake "UI" test to open the QUnit
  web page with Nightwatch, this had numerous limitations:

  - relies on fragile and unsupported DOM scraping to collect
    test results, which breaks between framework versions.
    ref #660.

  - severely limits debugging information for failing tests.

  - cannot easily be reproduced or debugged locally from the command-line
    as the Nightwatch config was pinned to Sauce Labs, and creating
    a local configuration is not easy because Nightwatch has a hard
    requirement for installing and running a WebDriver server.
    People usually do not have this installed and it's non-trivial
    to set up and keep working in the long term, and across multiple
    different software projects.

  - cannot easily be run in a secure container separate from your
    personal computer, thus putting personal data at risk.

  - lacks wider integration and plugins to enrich unit testing,
    such as test coverage reports.

  Using Karma means:
  - We can run 'npm test' locally during development and have it
    automatically run the tests in headless Firefox and Chrome
    and report back, all from the command-line.
  - The same exact same stack is also used in CI with SauceLabs
    for additional browser coverage (same as before).
  - It has no external dependencies other than the plain web
    browser itself. This means if you have a development container
    (e.g. based on Docker) that has Node.js + Firefox + Chromium,
    you can run the tests there without exposing anything from
    your personal computer, besides the current directory.
    <https://timotijhof.net/posts/2019/protect-yourself-from-npm/>
  - In a future change, we can plug in karma-coverage to generate
    a test coverage report, to submit to Codecov or Coveralls.
    ref #528.

* I have pinned the version of 'http-server' and 'nightwatch'
  in package.json so that these don't silently upgrade in a way
  that may introduce security issues or drop compatibility for
  the environment we currently support.

Fixes #653.

* Re-enable unit and UI tests in latest Edge

The tests were disabled after #499
due to an issue with the Edge version that was the default "edge" on
SauceLabs in May 2019 (not sure which version that was, the last pre-Chromium
Edge version was 44, which was passing, so perhaps SauceLabs defaulted
to a beta release, or used a much older version like 15-18?)

Now that Edge uses Chromium, try re-enabling the tests.

Fixes #502.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossroy mossroy mossroy approved these changes

Assignees

@Jaifroid Jaifroid

Labels
bug
Projects
None yet
Milestone
v2.6
Development

Successfully merging this pull request may close these issues.

It (rarely) happens to have both search results and an article displayed at the same time
2 participants
@Jaifroid @mossroy