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

Support date and number properties in facet search #1491

Closed
wants to merge 13 commits into from

Conversation

flammermann
Copy link
Contributor

@flammermann flammermann commented Dec 27, 2023

Implementation of #1489 and #1488

@flammermann flammermann mentioned this pull request Dec 28, 2023
@flammermann flammermann requested a review from davidbgk December 28, 2023 00:37
Copy link
Member

@yohanboniface yohanboniface left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this work!
I've made a few comments.
Could you also add some basic integration tests (in umap/test/integration) ? I'd say one basic case and one case where the data does not match the facet key. If you need help for this, please ask :)

Thanks again, that's a very nice contribution! :)

umap/static/umap/js/umap.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.features.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.features.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.controls.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.core.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
@yohanboniface
Copy link
Member

For the record, here is what the date form looks like currently in the facet form on my computer:

image

@davidbgk
Copy link
Contributor

For the record, here is what the date form looks like currently in the facet form on my computer:

Same here, Firefox support is not ideal for the moment:

Capture d’écran, le 2023-12-28 à 12 57 18

@flammermann
Copy link
Contributor Author

Sorry, for the late response. Thanks for all the comments, will look into them now. ;)

@flammermann
Copy link
Contributor Author

For the record, here is what the date form looks like currently in the facet form on my computer:

image

Yes, I found that too during my testing. It's not optimal but the user can use the date picker to choose the date and the keyboard (numbers or arrows) to enter/change the time. Also, I would expect/hope that Firefox eventually fixes their implementation to fully support datetime-local. Thus, I went with it despite the lack of support from Firefox.

Copy link
Member

@yohanboniface yohanboniface left a comment

Choose a reason for hiding this comment

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

👍

umap/static/umap/js/umap.core.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.js Outdated Show resolved Hide resolved
@flammermann
Copy link
Contributor Author

I refactored the code according to your suggestions. Support for number properties is now also given. I think the PR is in pretty good shape now. Thanks for the help and review! :)

@flammermann flammermann changed the title Support date properties in facet search Support date and number properties in facet search Jan 4, 2024
@yohanboniface
Copy link
Member

Could you also add some basic integration tests (in umap/test/integration) ? I'd say one basic case and one case where the data does not match the facet key.

I've pushed as basic facet search test, so you can base new ones on this :)

Copy link
Member

@yohanboniface yohanboniface left a comment

Choose a reason for hiding this comment

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

We are on good track, thanks! :)

umap/static/umap/js/umap.controls.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.controls.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.features.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
umap/static/umap/js/umap.forms.js Outdated Show resolved Hide resolved
@yohanboniface
Copy link
Member

hey @flammermann any time/interest in finishing this PR ? :)

@flammermann
Copy link
Contributor Author

hey @flammermann any time/interest in finishing this PR ? :)

Hi @yohanboniface, yes, I am still interested in finishing it. Unfortunately, my server crashed hard (harddrive/raid failure) shortly after my last comment/commit, so I could not continue. :/ But you got perfect timing, I finished recovery and rebuilding everything last Saturday and can now finally pick up this PR again. :)

Btw. I got lucky because I could export a lot of work on one of my maps that was not yet backed up properly from the still open browser tab. ;)

@yohanboniface
Copy link
Member

Sounds good!

Ideally, we'd need:

  • some integrations tests for basic cases: in umap/tests/integration/test_facets_browser.py, then make test or py.test -xvv umap/tests/integration/test_facets_browser.py to run only this file; for debugging PWDEBUG=1 py.test… to make playwright run tests in a headfull browser
  • some js tests for the calculateStepFromNumber util, in umap/static/umap/test/Util.js, then make testjs

Do you have time/energy for that ? Otherwise I'll do it next week.

@yohanboniface
Copy link
Member

Work continues in #1763

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