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

[#1711] [#1710] Make search filters conditional and add mobile toggle #763

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Sep 11, 2023

issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/1711
and also tackling issue #1710, because they are dependent: https://taiga.maykinmedia.nl/project/open-inwoner/task/1710

  • Not sure if I should add all 4 options with the first one being a checkbox to turn ALL filtering OFF immediately, or if I should just show the 3 options for Categories/Tags/Organizations, so this is what I built,

EDIT: removed the first filter-option and just left the other 3.

Screenshot 2023-09-14 at 18 54 30

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Merging #763 (b41bf06) into develop (1503f78) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #763   +/-   ##
========================================
  Coverage    93.56%   93.57%           
========================================
  Files          697      698    +1     
  Lines        24598    24605    +7     
========================================
+ Hits         23016    23023    +7     
  Misses        1582     1582           
Files Changed Coverage Δ
src/open_inwoner/configurations/admin.py 100.00% <ø> (ø)
src/open_inwoner/utils/context_processors.py 90.90% <ø> (ø)
...igrations/0052_siteconfiguration_search_filters.py 100.00% <100.00%> (ø)
src/open_inwoner/configurations/models.py 97.88% <100.00%> (+0.04%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jiromaykin jiromaykin force-pushed the feature/1711-hide-filters-when-unused branch from 410f8d7 to 30b263a Compare September 12, 2023 13:48
@jiromaykin jiromaykin changed the title [#1711] Make search filters conditional and add mobile toggle [#1711] [#1710] Make search filters conditional and add mobile toggle Sep 14, 2023
@jiromaykin jiromaykin force-pushed the feature/1711-hide-filters-when-unused branch 2 times, most recently from 24b5d9f to c089c80 Compare September 14, 2023 17:06
@jiromaykin jiromaykin marked this pull request as ready for review September 14, 2023 17:07
@Bartvaderkin
Copy link
Contributor

Bartvaderkin commented Sep 15, 2023

{% spaceless %} is not a free operation and a bit of a kludge so I'm having doubts it is a good idea to slap that everywhere for cosmetics on html source nobody looks at. You can get a long way with a bit of attention to the template and how to nest raw html and template tags.

Also note in existing cases {% spaceless %} was also used in the same vein as the <p class="p"> cargo cult: it was a kludge for a particular misunderstood problem which then gets replicated everywhere, but according to spec and everything there shouldn't be a problem.

src/open_inwoner/configurations/models.py Outdated Show resolved Hide resolved
src/open_inwoner/templates/pages/search.html Show resolved Hide resolved
src/open_inwoner/js/components/search/filter-mobile.js Outdated Show resolved Hide resolved
src/open_inwoner/js/components/search/filter-mobile.js Outdated Show resolved Hide resolved
Comment on lines 34 to 35
const filterToggles = document.querySelectorAll(FilterMobile.selector)
;[...filterToggles].forEach((filterToggle) => new FilterMobile(filterToggle))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's start removing this cargo-culted code pattern and just use forEach on the nodeList:

document.querySelectorAll(FilterMobile.selector).forEach((filterToggle) => new FilterMobile(filterToggle))

Later we'll move them to the index.

This goes for every querySelectorAll() & ;[...xxx] wart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bartvaderkin Okay, that would mean that, in future, we would need a bunch of imported Classes + initialized constants (with DOMContentLoaded event listener?) in the index file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in the future we'll add them all to the list in open_inwoner/js/components/index.js so we have a good overview of all the components, and they activate on page load as well as in htmx fragements.

src/open_inwoner/templates/pages/search.html Outdated Show resolved Hide resolved
@Bartvaderkin
Copy link
Contributor

I'm missing tests for this but I'll have to do more work on this and possibly restructure anyway.

@jiromaykin jiromaykin marked this pull request as draft September 18, 2023 15:14
@jiromaykin jiromaykin force-pushed the feature/1711-hide-filters-when-unused branch from c2c9d44 to 62f221d Compare September 19, 2023 08:00
@jiromaykin jiromaykin force-pushed the feature/1711-hide-filters-when-unused branch from 62f221d to b41bf06 Compare September 19, 2023 08:04
@jiromaykin jiromaykin marked this pull request as ready for review September 19, 2023 08:51
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I'll approve it and leave the filter-options.js for later when we address the components javascript.

@Bartvaderkin
Copy link
Contributor

I'm merging this for an interesting rebase.

@Bartvaderkin Bartvaderkin merged commit 09f11b9 into develop Sep 21, 2023
14 checks passed
@Bartvaderkin Bartvaderkin deleted the feature/1711-hide-filters-when-unused branch September 21, 2023 08:38
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