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

[#6486]budgeting-list/filterbar/search: adding freetext search components #4480

Merged
merged 1 commit into from
Sep 27, 2022
Merged

[#6486]budgeting-list/filterbar/search: adding freetext search components #4480

merged 1 commit into from
Sep 27, 2022

Conversation

khamui
Copy link
Contributor

@khamui khamui commented Sep 15, 2022

No description provided.

@khamui khamui force-pushed the kt-2022-09-search-filter branch from 8c37594 to c09bf8f Compare September 19, 2022 15:34
@khamui
Copy link
Contributor Author

khamui commented Sep 19, 2022 •
edited
Loading

  • check a11y
  • styling, polishing (mobile)
  • fix tests
  • make work with api
  • unify search input in mB and use bootstrap5 classes

@khamui khamui force-pushed the kt-2022-09-search-filter branch from c09bf8f to 89a10d3 Compare September 20, 2022 10:42
khamui
khamui commented Sep 20, 2022
View reviewed changes
meinberlin/assets/scss/components/_control-bar.scss
@@ -22,15 +22,9 @@
// @include grid-same-width(2);
}

> :last-child {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this only had effect on the react filterbar. so removing is fine i think.

@khamui khamui force-pushed the kt-2022-09-search-filter branch 2 times, most recently from 4bafef7 to ed7256c Compare September 20, 2022 12:43
@khamui khamui changed the title WIP budgeting-list/filterbar/search: adding freetext search components budgeting-list/filterbar/search: adding freetext search components Sep 20, 2022
@khamui
Copy link
Contributor Author

khamui commented Sep 20, 2022 •
edited
Loading

Note1:
Currently submitting search just appends &search=... to the request and removes it on clearing.

~~Note2: ~~
the last commit touches unrelated files, but were mentioned in the lighthouse scan (in case how i solved this is not ideal, i can remove the commit from this PR).

@khamui khamui changed the title budgeting-list/filterbar/search: adding freetext search components WIP budgeting-list/filterbar/search: adding freetext search components Sep 21, 2022
@khamui khamui force-pushed the kt-2022-09-search-filter branch from ed7256c to 2024fdf Compare September 21, 2022 07:58
@khamui khamui changed the title WIP budgeting-list/filterbar/search: adding freetext search components budgeting-list/filterbar/search: adding freetext search components Sep 21, 2022
@philli-m philli-m changed the title budgeting-list/filterbar/search: adding freetext search components [#6486]budgeting-list/filterbar/search: adding freetext search components Sep 22, 2022
philli-m
philli-m reviewed Sep 22, 2022
View reviewed changes
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

sorry changed name to include the story number, would be good if we can all start doing that more!
also my local environment is broken right now so couldn't actually install and look at it but added notes about the code, let me know if you want to talk through the possibility of refactoring

meinberlin/apps/budgeting/assets/FilterBarListMapSwitch.jsx Outdated Show resolved Hide resolved
meinberlin/apps/budgeting/assets/FilterBarSearch.jsx Outdated Show resolved Hide resolved
meinberlin/assets/scss/components/_filter_bar.scss Show resolved Hide resolved
meinberlin/assets/scss/components/_filter_bar.scss Outdated Show resolved Hide resolved
meinberlin/apps/votes/forms.py Outdated Show resolved Hide resolved
@khamui khamui changed the title [#6486]budgeting-list/filterbar/search: adding freetext search components WIP [#6486]budgeting-list/filterbar/search: adding freetext search components Sep 22, 2022
@khamui khamui force-pushed the kt-2022-09-search-filter branch from 2024fdf to 503799c Compare September 22, 2022 15:54
@khamui
Copy link
Contributor Author

khamui commented Sep 22, 2022 •
edited
Loading

to mention: the first commit is all commits before @phillimorland 's review, squashed together.

So now we are using Bootstrap5 classes for the search-filter inputs (most of them) and to make them responsive and layouted correctly i tried to utilize classes that we already have in use, instead of adding more. But in the end i had to add some overwrites to make it look how it is in the design.

@khamui khamui requested a review from philli-m September 22, 2022 16:09
@khamui khamui changed the title WIP [#6486]budgeting-list/filterbar/search: adding freetext search components [#6486]budgeting-list/filterbar/search: adding freetext search components Sep 22, 2022
philli-m
philli-m suggested changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

few comments about BEM also we discussed ensuring filters are the same everywhere and simplifying the overwrites, however that isn't apparent here, did you decide against it? i'm not sure if this may just increase the mess that's already there.

meinberlin/apps/budgeting/assets/FilterBar.jsx Outdated Show resolved Hide resolved
meinberlin/apps/budgeting/assets/FilterBar.jsx Outdated Show resolved Hide resolved
meinberlin/apps/budgeting/assets/FilterBar.jsx Outdated Show resolved Hide resolved
@khamui
Copy link
Contributor Author

khamui commented Sep 26, 2022 •
edited
Loading

few comments about BEM also we discussed ensuring filters are the same everywhere and simplifying the overwrites, however that isn't apparent here, did you decide against it? i'm not sure if this may just increase the mess that's already there.

No i didn't decide against the idea to use one search/freetext filter, are there more? those in A4 i didn't touch as we decided not to change those. This commit contains "other" places where I use the "new" search form:
1734c10

@phillimorland

@khamui khamui force-pushed the kt-2022-09-search-filter branch from 503799c to b93e285 Compare September 26, 2022 16:41
@khamui khamui requested a review from philli-m September 26, 2022 16:42
@khamui
Copy link
Contributor Author

khamui commented Sep 26, 2022

again i squashed all previous commits together into the first commit!

@khamui khamui force-pushed the kt-2022-09-search-filter branch from b93e285 to dc51037 Compare September 26, 2022 16:55
@philli-m philli-m self-assigned this Sep 27, 2022
philli-m
philli-m approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

super cool! nice and clean! :) @khamui do you want to squash all commits? then it's good to merge

budgeting-list/filterbar/search: adding freetext search components
5cb15ff 
budgeting-list/filterbar: rework to match new design behavior (tbd: search api & styling)

budgeting-list/filterbar: styling

budeting-list/filterbar/a11y: lighthouse scan

budgeting-list/filterbar/jstest: fix test

budgeting-list/filterbar/styles: using bs5 to style search forms

budgeting-list/filterbar/style: fixing FilterBarListMapSwitch.jsx

budgeting-list/filterbar/styles: using control-bar to layout (spacing) filterbar elements

search-forms: using bs5 searches for project search and map address search

component-lib: replace with bs5 version

budgeting-list/controlbar: changing filterbar component to controlbar

budgeting-list/controlbar/styles: using BEM names

budgeting-list/controlbar/styles: removing grow logic (tbc with Designer)
@khamui khamui force-pushed the kt-2022-09-search-filter branch from dc51037 to 5cb15ff Compare September 27, 2022 09:22
@khamui khamui requested a review from philli-m September 27, 2022 09:22
@philli-m philli-m merged commit 44dd04f into main Sep 27, 2022
@philli-m philli-m deleted the kt-2022-09-search-filter branch September 27, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philli-m philli-m philli-m approved these changes

@fuzzylogic2000 fuzzylogic2000 Awaiting requested review from fuzzylogic2000

@goapunk goapunk Awaiting requested review from goapunk

@Rineee Rineee Awaiting requested review from Rineee

Assignees

@philli-m philli-m

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@khamui @philli-m