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

Add record filter preset saving #1227

Merged

Conversation

kelanik8
Copy link
Contributor

The following changes are implemented

TODO: Summary

Changes in the user interface:

TODO: Add screenshots, recordings or remove this section

Checklist when submitting a final (!draft) PR

  • Commits are tidied up, squashed if needed and follow guidelines in CONTRIBUTING.md
  • Code builds
  • All existing tests pass
  • All new critical code is covered by tests
  • PR is linked to the relevant issue(s)
  • Rebased with the target branch

@kelanik8 kelanik8 requested a review from Fajfa May 24, 2023 12:04
@kelanik8 kelanik8 self-assigned this May 24, 2023
@kelanik8 kelanik8 linked an issue May 24, 2023 that may be closed by this pull request
Copy link
Member

@Fajfa Fajfa left a comment

Choose a reason for hiding this comment

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

Mistakenly selected Approve.

Copy link
Member

@Fajfa Fajfa left a comment

Choose a reason for hiding this comment

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

  1. If i close the save as preset naming modal I cannot save another preset
  2. Apply filter that you save as preset
  3. If I press reset, the Custom filter still stays
  4. This should be configurable, that means a new option in the record list configurator customFilterPresets, should be a checkbox in the filter preset section
  5. Make sure the record list filter component is also configurable to be able to hide that save as preset button since its not just used in the record list. And make sure its disabled in other places record list filter is used. We only want custom presets on record list block.

}

// Emit only value and not whole record with every filter
this.$emit('filter', this.processFilter())
Copy link
Member

Choose a reason for hiding this comment

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

This looks like you can just have a onSave function with (close = true, type = 'filter').
So that code doesn't repeat

name: 'CustomFilterPreset',

props: {
showCustomPresetFilterModal: {
Copy link
Member

Choose a reason for hiding this comment

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

can this be called visible, since based on where it is it is obvious it's the custom filter preset modal (component name)

default: false,
},

customFilter: {
Copy link
Member

Choose a reason for hiding this comment

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

this can just be called filter

},

watch: {
showCustomPresetFilterModal (val) {
Copy link
Member

Choose a reason for hiding this comment

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

Do the visible: { immediate: true, handler () } syntax please

},

created () {

Copy link
Member

Choose a reason for hiding this comment

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

No need for this if its empty

@@ -263,6 +263,12 @@ record:
recordList:
addRecord: Add
cancelSelection: Cancel
customFilter: Custom filter
customPresetFilter: Custom preset filter
Copy link
Member

Choose a reason for hiding this comment

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

i dont think you use this anywhere

customFilter: Custom filter
customPresetFilter: Custom preset filter
presetFilter:
reset: Reset
Copy link
Member

Choose a reason for hiding this comment

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

This also wont be needed since you don't need reset

@@ -263,6 +263,12 @@ record:
recordList:
addRecord: Add
cancelSelection: Cancel
customFilter: Custom filter
customPresetFilter: Custom preset filter
presetFilter:
Copy link
Member

Choose a reason for hiding this comment

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

filterPresets, presetFilter means something else

class="pt-0"
>
<b-form-group
:label="$t('recordList.presetFilter.filterName')"
Copy link
Member

Choose a reason for hiding this comment

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

label-class primary

>
<b-form-input
v-model="filterName"
required
Copy link
Member

Choose a reason for hiding this comment

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

the required here doesn't have any effect, you should however prevent saving(button should be disabled) if there is no filter name. You can also indicate its required by having a * in the label

Copy link
Member

@Fajfa Fajfa left a comment

Choose a reason for hiding this comment

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

  1. If i close the save as preset naming modal I cannot save another preset
  2. Saving as preset without applying filter doesn't apply and/or save the not applied one in the preset
  3. If I press reset, the Custom filter still stays
  4. This should be configurable, that means a new option in the record list configurator customFilterPresets, should be a checkbox in the filter preset section
  5. Make sure the record list filter component is also configurable to be able to hide that save as preset button since its not just used in the record list. And make sure its disabled in other places record list filter is used. We only want custom presets on record list block.

@katrinDY
Copy link
Contributor

katrinDY commented Jun 5, 2023

Issues found after QC check:

  • on clone/copy block, preset and "normal" filters are not copied into the new block config. The value of the option to set custom filters is cloned/copied correctly. Steps to reproduce: in a block click on copy block or copy block button -> in the newly copied/clone block notice how both of the filters are not present. This is not supported for now
  • "normal" filters are not shown after page refresh
  • after adding a preset filter, it’s shown in active filter section but not applied to the RL. Steps to reproduce: add a filter -> set it as preset -> remove the "normal filter" -> apply the preset filter -> notice it's not respected

@katrinDY
Copy link
Contributor

katrinDY commented Jun 6, 2023

Last thing:

  • if I add a preset filter, it's applied normally but the same filter is added as a normal filter. Please see the gif. Notice how after choosing the preset filter, a normal filter is added and after removing the normal filter, the preset filter is removed. Steps to reproduce: add a filter -> set it as prefilter -> remove the normal filter -> the preset filter is removed This is expected
    6

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-record-filter-preset-saving branch from c550f3f to aa28be2 Compare June 7, 2023 10:47
@kelanik8 kelanik8 merged commit aa28be2 into 2023.3.x Jun 7, 2023
1 check passed
@kelanik8 kelanik8 deleted the 2023.3.x-feature-add-record-filter-preset-saving branch June 7, 2023 10:54
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.

Add saving of record filter presets per user
3 participants