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

feat: kanban filters #17424

Closed
wants to merge 48 commits into from
Closed

feat: kanban filters #17424

wants to merge 48 commits into from

Conversation

hrwX
Copy link
Contributor

@hrwX hrwX commented Jul 6, 2022

  • Shows list filters for kanban board.

Bildschirmfoto 2022-07-09 um 12 54 00
Peek 2022-07-11 11-07

The normal list view has default filters and sorting. For kanban view, this was lacking. Our customer wants to use Kanban heavily, but would appreciate the same filtering and sorting options like in list view. – @barredterra

no-docs

@hrwX hrwX requested a review from a team as a code owner July 6, 2022 15:06
@hrwX hrwX requested review from surajshetty3416 and removed request for a team July 6, 2022 15:06
@hrwX
Copy link
Contributor Author

hrwX commented Jul 8, 2022

  • Unrelated tests failing in cypress

Copy link
Member

@shariquerik shariquerik left a comment

Choose a reason for hiding this comment

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

Bildschirmfoto 2022-07-09 um 12 54 00

It looks heavy because there are too many layers

image

This looks cleaner

@barredterra
Copy link
Collaborator

@shariquerik in your proposal the cards no longer pop out from the background, and we cannot easily see the columns. I agree that it looks cleaner, but maybe the UX is still worse. Shall we go ahead with the proposed change anyway?

frappe/public/scss/desk/kanban.scss Outdated Show resolved Hide resolved
@hrwX hrwX requested a review from shariquerik July 13, 2022 19:14
@barredterra
Copy link
Collaborator

barredterra commented Jul 17, 2022

@hrwX, I found a couple more issues (mostly not introduced by this PR, but maybe we can fix them anyway):

  • changing the filters and sort order should have a visible effect on the board!
  • Since we have a “sort” button now, can we remove all the logic related to updating the card order? How was this persisted anyway?
  • Not sure if the "Add Column" column makes any sense like it is implemented right now. Possible columns depend on the options of the underlying field (for example, status). From my understanding, it should not be a free text field, but offer the available options that don't already have a column (for example, to add back columns that have previously been archived).
  • "Add Column" should only be visible if the user has write permissions on this Kanban Board.
  • Column color picker should only be visible if the user has write permissions on this Kanban Board.
  • Don't try to save the current filters in Kanban Board if the user doesn't have write permissions on this board.
  • Column should have a default color, like when the status is displayed in list view. Not sure, if this even needs to be configurable.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #17424 (d4133ff) into develop (2b6fc68) will increase coverage by 0.26%.
The diff coverage is 75.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #17424      +/-   ##
===========================================
+ Coverage    62.39%   62.66%   +0.26%     
===========================================
  Files          744      744              
  Lines        68396    67283    -1113     
  Branches      5962     5965       +3     
===========================================
- Hits         42679    42161     -518     
+ Misses       22138    21640     -498     
+ Partials      3579     3482      -97     
Flag Coverage Δ
server-ui 29.74% <57.14%> (-0.13%) ⬇️
ui-tests 49.75% <81.25%> (+0.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@hrwX
Copy link
Contributor Author

hrwX commented Jul 21, 2022

@hrwX, I found a couple more issues (mostly not introduced by this PR, but maybe we can fix them anyway):

  • changing the filters and sort order should have a visible effect on the board!
  • Since we have a “sort” button now, can we remove all the logic related to updating the card order? How was this persisted anyway?
  • Not sure if the "Add Column" column makes any sense like it is implemented right now. Possible columns depend on the options of the underlying field (for example, status). From my understanding, it should not be a free text field, but offer the available options that don't already have a column (for example, to add back columns that have previously been archived).
  • "Add Column" should only be visible if the user has write permissions on this Kanban Board.
  • Column color picker should only be visible if the user has write permissions on this Kanban Board.
  • Don't try to save the current filters in Kanban Board if the user doesn't have write permissions on this board.
  • Column should have a default color, like when the status is displayed in list view. Not sure, if this even needs to be configurable.

@barredterra

  • For the archived columns and color, lets raise an issue and fix it in another PR, lets keep this one to only the introduction of standard filters. What do you think ?

@shariquerik
Copy link
Member

shariquerik commented Jul 25, 2022

@hrwX @barredterra
I was testing it. I saw some problems.

  1. When we change the filter Not Saved indicator should not come.
  2. Kanban is created based on some field like Status or Priority based on which sections are created so we should not have that field in the filter section.
  3. Now we have filters and the ability to sort which is good but it is breaking the functionality for which we are actually using kanban. We use kanban to move around the items from one section to another or prioritise them by moving up and down in the same section. If sort is applied you can drag and drop items but rearrange them according to the sort value applied. similarly, if we filter we will see limited items and if we drag items we don't know in which position they will go.

I think while any filter or sorting logic is applied we should not allow the user to move items in the same section.
And if we drag the item to a different section, the section should be highlighted saying something like "Sorting logic is applied the item will be placed accordingly".


Some UI fixes/feat - Unrelated to this PR, still mentioning them maybe fix them in this PR or open a new one.

  1. Like, comment and Assign + button are visible in all the items. We can show keep showing them if they have some data but for other items, let's show them when we hover over the item
  2. We should be able to drag the whole section
  3. If any doctype has 1000's of record kanban try to load them all which breaks it. Maybe we can lazy load the data based on scrolling in each section

@barredterra
Copy link
Collaborator

barredterra commented Jul 25, 2022

@shariquerik

(1) “Not Saved” indicator should come, but only if you have permissions to save.
(2) Yes, but I think that's a minor issue. If it can be easily done — okay. If not, it's much better that we can just reuse list filters as they are.
(3) “we should not allow the user to move items [within] the same [column]” – I agree.


prioritise them by moving up and down in the same section

This was the idea in the past, but it conflicts with the data model somehow, I think. For example, Task already has a priority field. If we allow drag and drop sorting in kanban, we would have a second field for tracking "Kanban priority", that's different from Task priority, and we allow sorting only by this one. IMO, It makes more sense to just sort by the original priority field (or any other one).

@shariquerik
Copy link
Member

shariquerik commented Jul 25, 2022

@barredterra

(1) “Not Saved” indicator should come, but only if you have permissions to save.

But why? either we have permission or not we are not updating records by setting filters

(2) Yes, but I think that's a minor issue. If it can be easily done — okay. If not, it's much better that we can just reuse list filters as they are.

👍🏻

(3) “we should not allow the user to move items [within] the same [column]” – I agree.

What about moving items to a different section?

@hrwX
Copy link
Contributor Author

hrwX commented Jul 25, 2022

@hrwX @barredterra I was testing it. I saw some problems.

  1. When we change the filter Not Saved indicator should not come.
  2. Kanban is created based on some field like Status or Priority based on which sections are created so we should not have that field in the filter section.
  3. Now we have filters and the ability to sort which is good but it is breaking the functionality for which we are actually using kanban. We use kanban to move around the items from one section to another or prioritise them by moving up and down in the same section. If sort is applied you can drag and drop items but rearrange them according to the sort value applied. similarly, if we filter we will see limited items and if we drag items we don't know in which position they will go.

I think while any filter or sorting logic is applied we should not allow the user to move items in the same section. And if we drag the item to a different section, the section should be highlighted saying something like "Sorting logic is applied the item will be placed accordingly".

Some UI fixes/feat - Unrelated to this PR, still mentioning them maybe fix them in this PR or open a new one.

  1. Like, comment and Assign + button are visible in all the items. We can show keep showing them if they have some data but for other items, let's show them when we hover over the item
  2. We should be able to drag the whole section
  3. If any doctype has 1000's of record kanban try to load them all which breaks it. Maybe we can lazy load the data based on scrolling in each section
  1. The Not Saved indicator would be removed based on the permissions.
  2. This wouldn't be in line with the customization that other views already provide the user. If the user chooses to filter results of only one section/column, he should be able to.
  3. Same section/column dragging would be disabled since it makes no sense now, but the user should be able to move tasks from one section/column to another.

UX issues

  1. Assign + has to be there since the user can assign a doc using kanban view itself
  2. Dragging the whole section, umm, kinda unnecessary as our kanban board is rendered off a field and its options, which in turn decide the priority. the user can always change it via customizations.
  3. Lazy loading, YES!

@barredterra
Copy link
Collaborator

@shariquerik

But why? either we have permission or not we are not updating records by setting filters

Default filters are saved in the Kanban Board. If I'm defining a new board, I can set and save the standard filters for this board.

@barredterra barredterra added the resolve-conflicts There seems to be conflict due to changes in current branch. Please resolve it to get your PR merged label Aug 4, 2022
@barredterra
Copy link
Collaborator

barredterra commented Aug 16, 2022

Bildschirmaufnahme.2022-08-17.um.00.02.31.mov

@hrwX seems to me like the card order doesn't get persisted yet.

@barredterra
Copy link
Collaborator

@hrwX the order is persisted now, but the filters no longer work 😄

@stale
Copy link

stale bot commented Sep 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Sep 30, 2022
@stale stale bot closed this Oct 3, 2022
@barredterra barredterra reopened this Oct 3, 2022
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Oct 3, 2022
@shariquerik
Copy link
Member

@hrwX & @barredterra anyone working on this?

@barredterra
Copy link
Collaborator

barredterra commented Oct 6, 2022

@shariquerik not really. Himanshu is not working with us anymore, so he's only able to contribute in his spare time.

I'm very uncomfortable with the implementation of the sorting logic that we're supposed to keep. Storing the sorted names of the documents in the Kanban Board doesn't make sense to me. What happens when there's a new record or an old one gets deleted? How can we reconcile the stored names with the real state in the database?

Any help would be much appreciated!

@shariquerik
Copy link
Member

I'm very uncomfortable with the implementation of the sorting logic that we're supposed to keep. Storing the sorted names of the documents in the Kanban Board doesn't make sense to me. What happens when there's a new record or an old one gets deleted? How can we reconcile the stored names with the real state in the database?

I think for the new record it's working but if any record is deleted we get an error (we can just ignore it while rendering and can remove it from the stored list).

Closing, for now, feel free to open it if anyone is working on it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
add-test-cases Add test case to validate fix or enhancement backport version-14-hotfix backport to version 14 squash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants