[OGUI-1760] Update search UI for object path and add filters #3065
Conversation
…not functional yet
…de launch config for Firefox debugging (extension + Firefox flags needed)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Adopted from Bookkeeping/lib/public/components/Filters/common/filtersPanelPopover.js | ||
| import { h, popover, PopoverAnchors, PopoverTriggerPreConfiguration } from '/js/src/index.js'; | ||
| // import { tooltip } from '../../common/popover/tooltip.js'; |
There was a problem hiding this comment.
Code that is not needed should not be left as comments
There was a problem hiding this comment.
I see you replied with "👍 " on this commend but the commented out code is still there.
QualityControl/public/pages/layoutListView/filtersPanelPopover.js
Outdated
Show resolved
Hide resolved
QualityControl/public/pages/layoutListView/filtersPanelPopover.js
Outdated
Show resolved
Hide resolved
QualityControl/public/pages/layoutListView/model/LayoutListModel.js
Outdated
Show resolved
Hide resolved
QualityControl/public/pages/layoutListView/model/SearchFilterModel.js
Outdated
Show resolved
Hide resolved
QualityControl/public/pages/layoutListView/model/SearchFilterModel.js
Outdated
Show resolved
Hide resolved
| ]), | ||
| }); | ||
| export default () => [ | ||
| h('.w-50.text-center', [h('b.f4', 'Layouts')]), |
There was a problem hiding this comment.
Currently you are not displaying the header Label Layouts anymore. This is because there has been a change in dev where the headers are now expected to be in format of centrlCol and rightCol.
I believe when you merged from dev, you did not realize this and kept your changes instead of merging them together.
Please have another look at the changes in this file
|
|
||
| // Adopted from Bookkeeping/lib/public/components/Filters/common/filtersPanelPopover.js | ||
| import { h, popover, PopoverAnchors, PopoverTriggerPreConfiguration } from '/js/src/index.js'; | ||
| // import { tooltip } from '../../common/popover/tooltip.js'; |
There was a problem hiding this comment.
I see you replied with "👍 " on this commend but the commented out code is still there.
| friendlyName: () => friendlyName ? friendlyName : key, | ||
| inputPlaceholder: () => inputPlaceholder ? inputPlaceholder : '', | ||
| getValue: () => value ? value : null, | ||
| // trim checks if value is a string value, test this |
There was a problem hiding this comment.
We should not just disable an eslint if inconvenient.
I think in this case, you cannot just remove the {} because of the other eslint rule not allowing assignments in arrow functions.
Thus, a cleaner solution is to apply the exception of one line function (contrary to the previous comments, I understand)
set: () => {
value = v:
}
There was a problem hiding this comment.
I understand it is actually the same as you initially added. It was an oversight from my side, forgetting about the assignment eslint rule, I apologise for that
| */ | ||
| stringifyActiveFiltersFriendly() { | ||
| let activeFilterText = 'Active filters: '; | ||
| if (this.allInActive()) { |
There was a problem hiding this comment.
If I understand this bit correctly, right now for this check, it works simply because there is only one filter present, but the moment we will add another one, the condition will not hold anymore.
I think is better to use the getAllActive and compare to 0, if no filter is active then no text is displayed.
| this.folders = new Map(); | ||
| this.searchFilterModel = new SearchFilterModel(); | ||
| this.searchFilterModel.observe(() => { | ||
| if (!this.searchFilterModel.allInActive()) { |
There was a problem hiding this comment.
I believe the filter should only send the active filters and not all of them. For example if we have 2 parameters, objectPath and tab, we only want to attach one of them to the request.
For testing purposes, try to register another filter
There was a problem hiding this comment.
Moreover, I think here the if condition should as well use the getActive like recommended in the other comment
| */ | ||
| this.filters = new Map(); // key -> filter instance | ||
| this.searchInput = ''; | ||
| this.register(createKeyValueFilter('objectPath', 'Object path', 'e.g. TPC')); |
There was a problem hiding this comment.
The registration should happen in the model using the search model as it is dependent on the page. In this case the LayoutListModel
I have JIRA issue created