Skip to content

feat: persist view filters and sorts on Update View button click#1290

Merged
charlesBochet merged 5 commits intomainfrom
feat/submit-view-filters-and-sorts
Aug 23, 2023
Merged

feat: persist view filters and sorts on Update View button click#1290
charlesBochet merged 5 commits intomainfrom
feat/submit-view-filters-and-sorts

Conversation

@thaisguigon
Copy link
Copy Markdown
Contributor

@thaisguigon thaisguigon commented Aug 22, 2023

Closes #1121
Closes #1123
Closes #1124
Closes #1289

@thaisguigon thaisguigon marked this pull request as ready for review August 22, 2023 15:12
@ergomake
Copy link
Copy Markdown

ergomake bot commented Aug 22, 2023

Hi 👋

Here's a preview environment 🚀

https://front-twentyhq-twenty-1290.env.ergomake.link

Environment Summary 📑

Container Source URL
front Dockerfile https://front-twentyhq-twenty-1290.env.ergomake.link
server Dockerfile https://server-twentyhq-twenty-1290.env.ergomake.link
postgres Dockerfile [not exposed - internal service]

Here are your environment's logs.

For questions or comments, join Discord.

Click here to disable Ergomake.

Copy link
Copy Markdown
Contributor

@emilienchvt emilienchvt left a comment

Choose a reason for hiding this comment

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

Nice one! A few quick comments on the naming of selectors and vocabulary diff between front and back.

When testing the feature I was surprised by a few behaviours:

  • When creating a new view, I’d expect it to be selected
  • After I create a filter on a new view, clicking on the “Filter button” toggles the filter&sorts bar which I find weird (Should enable new filter creation instead right ?)
  • No feedback when I “Update view” => I’d expect the button to be disabled Edit: addressed with #1293 (review)
  • When a view is selected, I can see a “cancel” button. I find it unclear what it does: it removes the filters and sorts of the view but does not change the “selected view” in the top bar.


import { savedFiltersScopedState } from './savedFiltersScopedState';

export const savedFiltersByKeyScopedState = selectorFamily({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

import { savedFiltersScopedState } from './savedFiltersScopedState';

export const savedFiltersByKeyScopedState = selectorFamily({
key: 'savedFiltersByKeyScopedState',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
key: 'savedFiltersByKeyScopedState',
key: 'savedFiltersByKeyScopedSelector',

Copy link
Copy Markdown
Contributor Author

@thaisguigon thaisguigon Aug 23, 2023

Choose a reason for hiding this comment

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

Done, thank you! I renamed all the other sorts and filters selectors as well.

import { savedSortsScopedState } from './savedSortsScopedState';

export const savedSortsByKeyScopedState = selectorFamily({
key: 'savedSortsByKeyScopedState',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
key: 'savedSortsByKeyScopedState',
key: 'savedSortsByKeyScopedSelector',

import { sortsScopedState } from './sortsScopedState';

export const sortsOrderByScopedState = selectorFamily({
key: 'sortsOrderByScopedState',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
key: 'sortsOrderByScopedState',
key: 'sortsOrderByScopedSelector',

return availableFilter
? ({
displayValue: viewFilter.displayValue ?? viewFilter.value,
field: viewFilter.key,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we have back and front agree on a naming key or field ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, I renamed field to key in types Filter, FilterDefinition and FilterDefinitionByEntity. I chose to keep key because it is called key in the SortType type as well.

@thaisguigon thaisguigon force-pushed the feat/submit-view-filters-and-sorts branch from 5c63958 to febc8c8 Compare August 23, 2023 15:02
@thaisguigon
Copy link
Copy Markdown
Contributor Author

thaisguigon commented Aug 23, 2023

Nice one! A few quick comments on the naming of selectors and vocabulary diff between front and back.

When testing the feature I was surprised by a few behaviours:

  • When creating a new view, I’d expect it to be selected
  • After I create a filter on a new view, clicking on the “Filter button” toggles the filter&sorts bar which I find weird (Should enable new filter creation instead right ?)
  • No feedback when I “Update view” => I’d expect the button to be disabled Edit: addressed with feat: disable Update View button if filters and sorts are up to date #1293 (review)
  • When a view is selected, I can see a “cancel” button. I find it unclear what it does: it removes the filters and sorts of the view but does not change the “selected view” in the top bar.

Good points!

  • I'll create an issue for this (switch to newly created view). Edit: On view creation, switch to new view #1297
  • As discussed with @charlesBochet: this will be addressed in an upcoming filters & sorts bar refactoring.
  • Already addressed in the PR you mentioned.
  • Not sure about this one, it might be addressed in the upcoming filters & sorts bar refactoring as well. Maybe labelling it differently ("Discard" for instance?) would make it clearer? @charlesBochet

@charlesBochet
Copy link
Copy Markdown
Member

  • one, it might be addressed in the upcoming filters & sorts bar refactoring as well. Maybe labelling it differently ("Discard" for instance?) would make it clear

I think we should rediscuss this once we have cleared the behaviors regarding the sort and filter chips bar. What opens it, what closes it? Then we can discuss Cancel button role. I would consider it separate from this PR

Copy link
Copy Markdown
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Great!

@charlesBochet charlesBochet merged commit 74ab014 into main Aug 23, 2023
@charlesBochet charlesBochet deleted the feat/submit-view-filters-and-sorts branch August 23, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants