vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: add button to clear active filters #924

Merged
konrad merged 25 commits from feat/clear-active-filter into main 2021-11-13 19:48:06 +00:00
Conversation 2 Commits 25 Files Changed 11 +227 -174
konrad commented 2021-10-31 15:25:28 +00:00
Owner
Copy Link
No description provided.
konrad requested review from dpschen 2021-10-31 15:25:35 +00:00
konrad reviewed 2021-10-31 15:29:48 +00:00
src/components/list/partials/filter-popup.vue Outdated
@ -9,0 +7,4 @@
{{ $t('filters.clear') }}
</x-button>
<filters
:class="{'is-open': visibleInternal}"
konrad commented 2021-10-31 15:26:57 +00:00
Author
Owner

The watcher in the component won't ever trigger if the component does not exist at all (with v-if). This hides it visually only.

The watcher in the component won't ever trigger if the component does not exist at all (with `v-if`). This hides it visually only.
dpschen commented 2021-10-31 23:39:02 +00:00
Member

Not sure which watcher you mean here. The one inside the <Filters>? If so => maybe a deep watcher helps.

Not sure which watcher you mean here. The one inside the `<Filters>`? If so => maybe a `deep` watcher helps.
dpschen commented 2021-10-31 23:44:07 +00:00
Member

I just realize that it also feels generally wrong to put this logic here:
Before the filter popup was just managing the filter and passing along the modelValue.
Probably this could have been easier if the FilterPopup would provide a slot for the popup.
Then maybe another one for the trigger … and then we basically can name the component just popup and integrate popper.js inside.
(that's kind of what my plan was for this anyway)

I just realize that it also feels generally wrong to put this logic here: Before the filter popup was just managing the filter and passing along the modelValue. Probably this could have been easier if the FilterPopup would provide a slot for the popup. Then maybe another one for the trigger … and then we basically can name the component just popup and integrate popper.js inside. (that's kind of what my plan was for this anyway)
konrad commented 2021-11-02 19:02:01 +00:00
Author
Owner

Not sure which watcher you mean here. The one inside the ? If so => maybe a deep watcher helps.

Exactly.

Probably this could have been easier if the FilterPopup would provide a slot for the popup.

So that it would not pass along the filter state? I'm actually not sure if this could have been easier in that case - I want to have the same "clear filters" button in all places where the filter popup is being used. I think moving this whole thing in a component simplifies that.

> Not sure which watcher you mean here. The one inside the <Filters>? If so => maybe a deep watcher helps. Exactly. > Probably this could have been easier if the FilterPopup would provide a slot for the popup. So that it would not pass along the filter state? I'm actually not sure if this could have been easier in that case - I want to have the same "clear filters" button in all places where the filter popup is being used. I think moving this whole thing in a component simplifies that.
dpschen commented 2021-11-02 19:28:25 +00:00
Member

I think I understand what you mean now. I meant something similar to this:

<!-- new Filter.vue -->
<template>
	<!-- put in the new clearFilters button -->
	<XBbutton
		v-if="hasFilters"
		type="secondary"
		@click="clearFilters"
	>
		{{ $t('filters.clear') }}
	</XButton>
    
    
    <!-- // new popup component, manages `open` state internally; no need for the state to be outside, includes logic of `closeWhenClickedOutside` -->
    <popup>
        <template #trigger="{isOpen, toggle}">
        	<!-- this button is currently inside Kanban, gantt-component, ... -->
            <button
                @click="toggle()"
            >open filters</button>
        </template>

        <template>
            <!-- put in here the complete content of the current Filters.vue -->
        </template>
    </popup>
</template>

<!-- [...] -->
I think I understand what you mean now. I meant something similar to this: ```vue <!-- new Filter.vue --> <template> <!-- put in the new clearFilters button --> <XBbutton v-if="hasFilters" type="secondary" @click="clearFilters" > {{ $t('filters.clear') }} </XButton> <!-- // new popup component, manages `open` state internally; no need for the state to be outside, includes logic of `closeWhenClickedOutside` --> <popup> <template #trigger="{isOpen, toggle}"> <!-- this button is currently inside Kanban, gantt-component, ... --> <button @click="toggle()" >open filters</button> </template> <template> <!-- put in here the complete content of the current Filters.vue --> </template> </popup> </template> <!-- [...] --> ```
konrad commented 2021-11-02 20:13:55 +00:00
Author
Owner

So basically moving the button and the open state to the filter popup component?

So basically moving the button and the open state to the filter popup component?
konrad commented 2021-11-02 20:14:27 +00:00
Author
Owner

Both buttons, clear and toggle, that is

Both buttons, clear and toggle, that is
konrad commented 2021-11-02 21:00:06 +00:00
Author
Owner

I put something together in 86efb07f09. That seems to work. Is that what you had in mind?

I put something together in 86efb07f098392f50ebf607c070f321d34697205. That seems to work. Is that what you had in mind?
dpschen commented 2021-11-02 21:00:59 +00:00
Member

yes / no:

  • the popup is becoming it's own component that just contains general popup logic, aka not filter specific.
  • it provides slots for the button and the content. I made this the default slot but maybe #content would have been better.
  • With the slots it provides methods to open / close to the popup and show it's current state — isOpen can be used to control active classes at the button
  • it handles the isOpen state internally.
  • the popup could also be used for: showActiveColumnsFilter in Table.vue

I did though move the reset button together with the other stuff from the current filters.vue.

So if you would have written:

So basically moving the button and the open state to the filter popup component?

Then the answer would be yes ✅

yes / no: - the popup is becoming it's own component that just contains _general_ popup logic, aka not filter specific. - it provides slots for the button and the content. I made this the default slot but maybe `#content` would have been better. - With the slots it provides methods to open / close to the popup and show it's current state — `isOpen` can be used to control active classes at the button - it handles the isOpen state internally. - the popup could also be used for: `showActiveColumnsFilter` in `Table.vue` ---- I did though move the reset button together with the other stuff from the current filters.vue. --- So if you would have written: > So basically moving the button and the open state to the filter ~~popup~~ component? Then the answer would be yes ✅
konrad marked this conversation as resolved
src/components/list/partials/filter-popup.vue Outdated
@ -43,0 +56,4 @@
}
const def = {...defaultParams(), s: null}
return JSON.stringify(filterParams) !== JSON.stringify(def)
konrad commented 2021-10-31 15:27:13 +00:00
Author
Owner

This feels very hacky...

This feels very hacky...
dpschen commented 2021-11-01 00:07:53 +00:00
Member

Here specifically:
You can remove a parameter by destructuring it and then just use the rest:

const {page, ...value} = this.value

// just continue to use `value`

JSON.stringify(filterParams) !== JSON.stringify(def)

👆 this line is fine – in case you included that in what you meant feels hacky – as long as it works

All together my feeling is that hasFilters should be a computed that is defined where the filters state is saved.

Here specifically: You can remove a parameter by destructuring it and then just use the rest: ```js const {page, ...value} = this.value // just continue to use `value` ``` ```js JSON.stringify(filterParams) !== JSON.stringify(def) ``` 👆 this line is fine – in case you included that in what you meant feels hacky – as long as it works All together my feeling is that `hasFilters` should be a computed that is defined where the filters state is saved.
konrad commented 2021-11-02 19:04:19 +00:00
Author
Owner

You can remove a parameter by destructuring it and then just use the rest:

You mean in that case value would contain everything from this.value without page?

> You can remove a parameter by destructuring it and then just use the rest: You mean in that case `value` would contain everything from `this.value` without `page`?
👍 1
konrad commented 2021-11-02 21:04:09 +00:00
Author
Owner

Changed.

Changed.
konrad marked this conversation as resolved
src/components/tasks/mixins/taskList.js Outdated
@ -10,6 +10,17 @@ const DEFAULT_PARAMS = {
filter_concat: 'and',
}
export const defaultParams = () => {
konrad commented 2021-10-31 15:29:33 +00:00
Author
Owner

I only got this working once I created this factory thing. Every time I'd use the constant directly, the params variable in the components and the value of this const were always the same. I suspect this is due to some JS-reference passing stuff.

I only got this working once I created this factory thing. Every time I'd use the constant directly, the params variable in the components and the value of this const were always the same. I suspect this is due to some JS-reference passing stuff.
dpschen commented 2021-11-01 00:24:49 +00:00
Member

Makes sense. I didn't use a getter function since it seemed to me more "JSONy".
How about:

const DEFAULT_PARAMS = {
	sort_by: ['position', 'id'],
	order_by: ['asc', 'desc'],
	filter_by: ['done'],
	filter_value: ['false'],
	filter_comparator: ['equals'],
	filter_concat: 'and',
}

export const getDefaultParams = () => DEFAULT_PARAMS
Makes sense. I didn't use a getter function since it seemed to me more "JSONy". How about: ```js const DEFAULT_PARAMS = { sort_by: ['position', 'id'], order_by: ['asc', 'desc'], filter_by: ['done'], filter_value: ['false'], filter_comparator: ['equals'], filter_concat: 'and', } export const getDefaultParams = () => DEFAULT_PARAMS ```
konrad commented 2021-11-02 19:05:56 +00:00
Author
Owner

I tried that, it did not work unfortunately. My theory is that the function only returns a reference to the const which is then changed. And because everything holds a copy to that same const reference, it is changed everywhere simultaniously.

I tried that, it did not work unfortunately. My theory is that the function only returns a reference to the const which is then changed. And because everything holds a copy to that same const reference, it is changed everywhere simultaniously.
dpschen commented 2021-11-02 19:10:56 +00:00
Member

Maybe:

export const getDefaultParams = () => { ...DEFAULT_PARAMS }
Maybe: ```js export const getDefaultParams = () => { ...DEFAULT_PARAMS } ```
konrad commented 2021-11-02 19:27:55 +00:00
Author
Owner

That seems to work.

That seems to work.
👍 1
konrad commented 2021-11-02 21:00:50 +00:00
Author
Owner

Actually, it does not seem to work. Reverted...

Actually, it does not seem to work. Reverted...
konrad marked this conversation as resolved
dpschen reviewed 2021-11-01 00:16:57 +00:00
src/components/list/partials/filters.vue Outdated
@ -467,3 +467,1 @@
if (foundDone === false) {
this.filters.done = true
}
this.filters.done = foundDone === false
dpschen commented 2021-11-01 00:16:57 +00:00
Member 
prepareDone() {
	// Set filters.done based on params
	if (typeof this.params.filter_by === 'undefined') {
		return
	}

	this.filter.done = this.params.filter_by.some((f) => f === 'done') === false
},
```js prepareDone() { // Set filters.done based on params if (typeof this.params.filter_by === 'undefined') { return } this.filter.done = this.params.filter_by.some((f) => f === 'done') === false }, ```
konrad commented 2021-11-02 19:07:00 +00:00
Author
Owner

Done.

Done.
👍 1
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-11-01 00:38:56 +00:00
konrad force-pushed feat/clear-active-filter from 5d399e05a9 to dc02c09f19 2021-11-02 19:02:32 +00:00 Compare
konrad added 1 commit 2021-11-02 19:06:58 +00:00
continuous-integration/drone/pr Build is failing Details
df1a76d529
chore: simplify done filter check
konrad added 2 commits 2021-11-02 19:27:42 +00:00
continuous-integration/drone/pr Build is failing Details
7207c452f3
chore: use const for default params
konrad added 1 commit 2021-11-02 19:56:58 +00:00
continuous-integration/drone/pr Build is failing Details
566fe77b02
fix: defaultParams function name
konrad added 2 commits 2021-11-02 20:11:26 +00:00
continuous-integration/drone/pr Build is passing Details
9250f4e76b
fix: column active test
konrad added 1 commit 2021-11-02 20:59:25 +00:00
konrad added 1 commit 2021-11-02 21:04:04 +00:00
continuous-integration/drone/pr Build is failing Details
c4b9bc5942
chore: simplify using only some filter values
konrad added 1 commit 2021-11-02 21:09:25 +00:00
continuous-integration/drone/pr Build is passing Details
bc7c89d859
fix: lint
konrad added 1 commit 2021-11-02 21:12:59 +00:00
continuous-integration/drone/pr Build is passing Details
0fbe83c362
chore: put ref on popup wrapper div instead of slot
dpschen reviewed 2021-11-02 21:16:42 +00:00
src/components/misc/popup.vue Outdated
@ -0,0 +54,4 @@
overflow: hidden;
position: absolute;
top: 1rem;
margin: 0 !important;
dpschen commented 2021-11-02 21:16:42 +00:00
Member

margin should be defined by the popup-content

margin should be defined by the popup-content
konrad commented 2021-11-02 21:50:19 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-02 21:17:39 +00:00
continuous-integration/drone/pr Build is failing Details
ec927d6564
feat: move active columns filter to new popup component
konrad added 1 commit 2021-11-02 21:19:21 +00:00
continuous-integration/drone/pr Build is failing Details
5bf288a066
feat: rename default slot to content
konrad added 1 commit 2021-11-02 21:31:00 +00:00
continuous-integration/drone/pr Build is failing Details
e29a19f218
fix: restrict filter params in comparison
konrad added 1 commit 2021-11-02 21:37:02 +00:00
continuous-integration/drone/pr Build is passing Details
fa25fbccd0
fix: close when clicked outside
konrad added 1 commit 2021-11-02 21:40:37 +00:00
continuous-integration/drone/pr Build is passing Details
cff2a0bf6f
chore: use <script setup>
konrad added 1 commit 2021-11-02 21:50:14 +00:00
continuous-integration/drone/pr Build is passing Details
c040620185
chore: move margin outside of the popup component
konrad removed their assignment 2021-11-03 19:45:15 +00:00
konrad commented 2021-11-09 22:58:16 +00:00
Author
Owner
Copy Link

@dpschen Could you give this another review?

@dpschen Could you give this another review?
👍 1
dpschen was assigned by konrad 2021-11-09 22:58:25 +00:00
dpschen requested changes 2021-11-11 01:07:13 +00:00
src/components/list/partials/filter-popup.vue Outdated
@ -11,3 +30,3 @@
<script>
import {closeWhenClickedOutside} from '@/helpers/closeWhenClickedOutside'
import Filters from '../../../components/list/partials/filters'
import {getDefaultParams} from '../../tasks/mixins/taskList'
dpschen commented 2021-11-11 01:00:40 +00:00
Member

use @

use `@`
konrad commented 2021-11-13 15:42:49 +00:00
Author
Owner

Done. I think we should put this in the linter at some point, I keep forgetting it... and webstorm always seems to use relative paths by default.

Done. I think we should put this in the linter at some point, I keep forgetting it... and webstorm always seems to use relative paths by default.
konrad marked this conversation as resolved
src/components/list/partials/filter-popup.vue Outdated
@ -75,0 +89,4 @@
<style scoped lang="scss">
.filter-popup {
margin: 0 !important;
dpschen commented 2021-11-11 01:01:34 +00:00
Member

Why the !important?

Why the `!important`?
konrad commented 2021-11-13 15:52:18 +00:00
Author
Owner

There was a margin set for .filter-container .card (in list.scss). I've removed that one and the !important statements.

There was a margin set for `.filter-container .card` (in `list.scss`). I've removed that one and the `!important` statements.
konrad marked this conversation as resolved
src/components/misc/popup.vue Outdated
@ -0,0 +16,4 @@
open.value = !open.value
}
const hidePopup = e => {
dpschen commented 2021-11-11 01:03:55 +00:00
Member

Picky: use function. Makes it more clear what a "method" is (now simply function) next to a ref.

Picky: use `function`. Makes it more clear what a "method" is (now simply function) next to a ref.
konrad commented 2021-11-13 15:53:14 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
src/views/list/views/Table.vue Outdated
@ -200,10 +196,12 @@ import Sort from '../../../components/tasks/partials/sort'
import {saveListView} from '@/helpers/saveListView'
import FilterPopup from '@/components/list/partials/filter-popup.vue'
import Pagination from '@/components/misc/pagination.vue'
import Popup from '../../../components/misc/popup'
dpschen commented 2021-11-11 01:05:50 +00:00
Member

use @

use `@`
konrad marked this conversation as resolved
src/views/list/views/Table.vue Outdated
@ -325,1 +322,4 @@
}
.columns-filter {
margin: 0 !important;
dpschen commented 2021-11-11 01:06:18 +00:00
Member

why !important?

why `!important`?
konrad marked this conversation as resolved
src/views/list/views/Table.vue
@ -326,0 +324,4 @@
.columns-filter {
margin: 0 !important;
&.is-open {
dpschen commented 2021-11-11 01:07:07 +00:00
Member

we shouldn't style elements from outside.
Why do we need to position this from outside via margin?

we shouldn't style elements from outside. Why do we need to position this from outside via margin?
konrad commented 2021-11-13 15:56:39 +00:00
Author
Owner

Because by default, there's no margin. And that would let it flow in the other buttons instead.

How would you solve it? Where would you apply the margin instead?

Because by default, there's no margin. And that would let it flow in the other buttons instead. How would you solve it? Where would you apply the margin instead?
dpschen commented 2021-11-13 19:20:58 +00:00
Member

Okay I just tried to fix what I meant but realized there are much more changes needed for that. I think that would be out of the scope of this branch and better fitted when we add a popper.js based popup =)

Okay I just tried to fix what I meant but realized there are much more changes needed for that. I think that would be out of the scope of this branch and better fitted when we add a popper.js based popup =)
👍 1
konrad commented 2021-11-13 19:23:11 +00:00
Author
Owner

Sounds like a topic for another day :)

Sounds like a topic for another day :)
👍 1
dpschen commented 2021-11-13 19:23:56 +00:00
Member

I think that was the last =)

I think that was the last =)
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-11-11 01:07:29 +00:00
dpschen removed their assignment 2021-11-11 01:07:29 +00:00
konrad added 2 commits 2021-11-13 15:42:07 +00:00
continuous-integration/drone/pr Build is failing Details
300fbe4955
chore: use @ for imports
konrad added 1 commit 2021-11-13 15:48:57 +00:00
continuous-integration/drone/pr Build was killed Details
4fcd456626
fix: import paths
konrad added 1 commit 2021-11-13 15:51:21 +00:00
continuous-integration/drone/pr Build was killed Details
16ccbcb2c2
chore: remove !important
konrad added 1 commit 2021-11-13 15:53:09 +00:00
continuous-integration/drone/pr Build was killed Details
7dac2275ab
chore: use function
konrad added 1 commit 2021-11-13 15:54:58 +00:00
continuous-integration/drone/pr Build is passing Details
9fa0592ee3
chore: remove !important
frederick commented 2021-11-13 16:04:28 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://924-featclear-active-filter--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://924-featclear-active-filter--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen approved these changes 2021-11-13 19:38:15 +00:00
konrad merged commit 31f0c384ac into main 2021-11-13 19:48:06 +00:00
konrad deleted branch feat/clear-active-filter 2021-11-13 19:48:06 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#924
No description provided.
Delete Branch "feat/clear-active-filter"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?