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

Browse items by category #10163

Merged
merged 11 commits into from
Feb 3, 2022
Merged

Conversation

Rom1-B
Copy link
Contributor

@Rom1-B Rom1-B commented Dec 17, 2021

Browse items by category, as already exists for kb items

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !22497

image

image

@Rom1-B Rom1-B force-pushed the support_22497_treeview branch from d4a1a8c to 1fc1581 Compare December 17, 2021 09:09
@cedric-anne cedric-anne added this to the 10.0.0 milestone Jan 4, 2022
@orthagh orthagh force-pushed the support_22497_treeview branch from aede5ad to 63ce675 Compare January 4, 2022 13:36
@Rom1-B Rom1-B force-pushed the support_22497_treeview branch from 23f4ffd to 5f63198 Compare January 10, 2022 08:37
@Rom1-B Rom1-B force-pushed the support_22497_treeview branch from 5f63198 to 12c9925 Compare January 18, 2022 11:14
@Rom1-B
Copy link
Contributor Author

Rom1-B commented Jan 19, 2022

It should be ok now

@cedric-anne cedric-anne force-pushed the support_22497_treeview branch from 68e94bf to 3962f2c Compare January 20, 2022 11:45
@cedric-anne cedric-anne changed the title feat(document): browse by category Browse items by category Jan 20, 2022
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

For me, this PR is almost ready to be merged.

The tree display should be improved. On my browser, the left border seems to be missing
image

Also, I still have some edge cases that may require a fix.

  1. The selected category is not stored, so when the page is refresh (manually, or when user changes displayed columns), the previously selected category is lost and the items without any category are displayed.

  2. When adding a new criterion, or when switching to "trashbin" view, page is refreshed, and selected category is lost. Even if it was stored, it may be empty when using new criteria, so it may not be present in the tree.

@cedric-anne
Copy link
Member

cedric-anne commented Jan 21, 2022

We do a functionnal review with @orthagh and here are our comments.

  1. This feature should be remove on tickets for now, we may reactivate it on a future GLPI version, but with some enhancements. No need to clean the code, just remove usage of Glpi\Features\TreeBrowse in CommonITILObject.

  2. When the selected category contains no elements, it should still be displayed and selected in the tree, so user will not be "lost" when changing criteria result in having no elements in current category.

  3. When the browse view is displayed, the javascript logic behind the search view seems not correctly handled. Indeed, changing criteria triggers a full page refresh, changing the number of items displayed is not working, ...

  4. The tree seems to be build using a limited search results, so the number of items in category tree is not correct. For instance, if there is 10 categories with 1 item in each, they will be all displayed when at least 10 rows/page are displayed, but the tree will be limited to 5 categories when 5 rows/page are displayed.

  5. When pagination is used, a new criteria Characteristics - Heading > is > ... is added to search criteria. So once page is reload, tree contains only the current category and is not browsable anymore.

  6. On browse view, a scrollbar is always added, even when there is only a few items displayed. Removing the height: 100% on #tree_browse style seems to fix this, but it may have side effects on KB view.

  7. The category selected by default (Without category) has no "active" style, so user may not understand that results are filtered.

@Rom1-B Rom1-B force-pushed the support_22497_treeview branch 2 times, most recently from fbf1e78 to 2dacaab Compare January 26, 2022 11:39
@cconard96
Copy link
Contributor

It looks like when switching to the tree browse view, it replaces the search-display-data element and doesn't use the display_data.html.twig template so the JS isn't initialized for fluid search.
I think a new view would have to be created, possibly extending js/modules/Search/Table.js to handle it.

I can work on fluid search compatibility if you want.

@cedric-anne
Copy link
Member

@Rom1-B Can you check if 817c07d fixes latests issues and integrate it in this PR ?

@Rom1-B Rom1-B force-pushed the support_22497_treeview branch from 2dacaab to 508f899 Compare February 2, 2022 13:06
@cedric-anne
Copy link
Member

This is still not OK.

When you use any of the search actions (change a criteria, use the pagination, ...), the category filter is not applied anymore.

@cconard96
Copy link
Contributor

I thought so so I made my PR a draft. It will need some changes on the JS side to inject the search criteria.

@cedric-anne
Copy link
Member

@cconard96 Do not hesitate to push your fixes here.

@cconard96
Copy link
Contributor

I'm not sure if I can. I've tried for other PRs in the past but I don't have access.

I thought only repo maintainers could push to other people's PRs.

@Rom1-B Rom1-B force-pushed the support_22497_treeview branch from 508f899 to af4d34b Compare February 2, 2022 14:42
@Rom1-B
Copy link
Contributor Author

Rom1-B commented Feb 2, 2022

It is not yet OK. The criteria are not applied immediately, you have to apply twice.
I must be missing something.

@cconard96
Copy link
Contributor

I thought so so I made my PR a draft. It will need some changes on the JS side to inject the search criteria.

With the category tree filtering results after the regular search is applied, I'm not sure of the correct fix.

@cedric-anne cedric-anne dismissed their stale review February 3, 2022 13:00

Changes were made.

@cedric-anne cedric-anne self-requested a review February 3, 2022 13:00
@cedric-anne
Copy link
Member

As seen internally, I merge this to be able to integrate it in 10.0.0-rc. Latest bug have to be fixed in another PR soon.

@cedric-anne cedric-anne merged commit 2282eb6 into glpi-project:master Feb 3, 2022
@Rom1-B Rom1-B deleted the support_22497_treeview branch February 9, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants