view toolbar: add search & filter functionality#2370
Conversation
tests/e2e/specs/tree.cy.js
Outdated
| // it('Provides a select all functionality', () => { | ||
| // cy.visit('/#/tree/one') | ||
| // cy.get('[data-cy="control-taskStateFilter"]') | ||
| // .get('.v-list-item--active') | ||
| // .should('have.length', 0) | ||
| // cy.get('[data-cy="control-taskStateFilter"]') | ||
| // .click() | ||
| // .get('[data-cy=task-filter-select-all]') | ||
| // .click() | ||
| // cy.get('[data-cy="control-taskStateFilter"]') | ||
| // .get('.v-list-item--active') | ||
| // .should('have.length', 8) | ||
| // }) |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm OK with leaving this out of this PR. Perhaps add a comment to explain why this is commented out?
There was a problem hiding this comment.
Actually it's just occurred to me a deselect-all button would be useful?
src/views/Tree.vue
Outdated
| .toolbar { | ||
| position: sticky; | ||
| top: 0; | ||
| background: white; |
There was a problem hiding this comment.
Any better ideas than white? Some SCSS variable for the background color?
95ea7cd to
7e782da
Compare
| const numTasks = sortedTasks.length | ||
| describe('Options save state', () => { | ||
| beforeEach(() => { | ||
| localStorage.defaultView = 'Analysis' |
There was a problem hiding this comment.
Some of the tests in this file (and elsewhere) are searching for menu items in a relatively naive way.
Adding the task state menu to the tree view caused these tests to fail as they were selecting tree view filter controls from the other tab.
The tree view does not need to be open for these tests, so rather than opening the analysis view over the top (and deal with the consequences of having the tree view underneath), I just set it up to open the analysis view by default.
| selectOption('#c-gantt-filter-job-name', 'c3') | ||
| // Set task times filter option to something other than default ('Total times') | ||
| selectOption('#c-gantt-filter-job-timings', 'Queue') | ||
| selectOption('#c-gantt-filter-job-timings', 'Queue times') |
There was a problem hiding this comment.
Queue also matched the task state Queued item, expanded to Queue times to disambiguate.
| it('should show a summary of tasks if the number of selected items is greater than the maximum limit', () => { | ||
| cy.visit('/#/tree/one') | ||
| cy.get('[data-cy="filter task state"]') | ||
| .click() | ||
| // eslint-disable-next-line no-lone-blocks | ||
| TaskState.enumValues.forEach(state => { | ||
| cy.get('.v-list-item') | ||
| .contains(state.name) | ||
| .click({ force: true }) | ||
| }) | ||
| // Click outside to close dropdown | ||
| cy.get('noscript') | ||
| .click({ force: true }) | ||
| cy.get('[data-cy="filter task state"]') | ||
| .contains('.v-select__selection', '(+') | ||
| }) | ||
|
|
There was a problem hiding this comment.
Functionality removal/modification, the task state filter no longer shows the filtered states, it just highlights the icon as "active" if filters are in play.
7e782da to
749981e
Compare
wxtim
left a comment
There was a problem hiding this comment.
Looks good 😄 - Can you address my comments (may well not be blockers).
MetRonnie
left a comment
There was a problem hiding this comment.
Partial review.
The task state filtering is well thought out, however I found one slight annoyance IMO with the waiting modifiers: clicking the drop down the first time only selects waiting without expanding the modifiers. After this first click does the expand/collapse work. Also it seems the modifiers remain selected after waiting is deselected.
20251204-1755-54.1473635.mp4
Ideally IMO the expansion would work off the bat, and clicking one of the modifiers would automatically select Waiting if it was not already
There was a problem hiding this comment.
(Discussion to possibly take into another issue, not hold up this PR)
IMO this is becoming increasingly difficult to use. This single component controls the presentation and business logic of several input types, making it difficult to reason about how they are working.
In future, I think it would be worth extracting out the input types that are used here into their own SFCs, leaving this as a component that controls presentation and vuetify defaults, with a slot for allowing each view to set its controls (the more conventional way of using Vue components).
While I understand the idea behind this existing implementation (#1809 (comment)), the main advantage being having an object that contains the configuration that can be re-used in the settings page, this can still be achieved under what I'm proposing.
What do you think?
There was a problem hiding this comment.
For another issue if you are sufficiently comfortable with this as it is here.
As it stands this is right at the edge of suitability for the current approach, agreed. Happy to look towards restructuring this code going forwards, e.g, something along the lines of the GraphQL form generator. Also if we end up taking this further in the future, a more model-orientated approach (hopefully removing the setOption callback) would be great.
I wouldn't like to see the complexity being decentralised back into the views (which one interpretation of the slots suggestion may result in), but there are defo other ways to do this.
With this PR I think the toolbar should do everything we need from it now and for the foreseeable (just need to add search & de-select all buttons for the graph view PR). So I don't think we should look into refactoring in the short/mid term (time better spent elsewhere). But if / when requirements start to become more advanced...
58ebcf1 to
d8c27bf
Compare
TODO:
|
2d2ce01 to
0ac3128
Compare
wxtim
left a comment
There was a problem hiding this comment.
Legit - tested as working. Code looks good. :)
0ac3128 to
ddd6bc3
Compare
MetRonnie
left a comment
There was a problem hiding this comment.
Suggestion for simplifying some stuff at oliver-sanders#122
Couple of points on the glob matching:
- How are users going to know that you can use globs?
- Glob matching is not working in a weird way for the table view:
ddd6bc3 to
90841e3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
90841e3 to
fc38f0c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
wxtim
left a comment
There was a problem hiding this comment.
Reviewed changes since last review & happy
MetRonnie
left a comment
There was a problem hiding this comment.
Are you working on a "deselect all"/"clear filters" button? (#2370 (comment))
On it. |
c1764a0 to
14fc4ec
Compare
This comment was marked as outdated.
This comment was marked as outdated.
14fc4ec to
9028ce4
Compare
|
Addressed comments (bar one). Fixed a small pre-existing issue where the search feature was matching family names in the tree view even when families were turned off - 9028ce4 Added a filter "reset" button - https://github.com/cylc/cylc-ui/compare/14fc4ecea00adac5cb851b760a776b29d6af403e..9028ce44997134b59b8763d29cd5c7cb5969f77a |
| it('filters task states', () => { | ||
| mountToolbar([ | ||
| { | ||
| title: 'Group 1', | ||
| controls: [ | ||
| { | ||
| title: 'Filter', | ||
| action: 'taskStateFilter', | ||
| value: [], | ||
| key: 'filter' | ||
| } | ||
| ] | ||
| }, | ||
| ]) |
There was a problem hiding this comment.
In recent changes I have fixed a reactivity issue (which means that the eager is no longer needed).
Unfortunately, that has broken this test. In order for this to work, we need to implement a setOption callback like we do in each view, i.e:
<ViewToolbar @setOption="setOption" />I think the solution should be something along the lines of (see listeners in the Info View tests):
import { reactive } from 'vue'
const model = reactive([])
mountToolbar({
props: { groups: [...] },
listeners: { setOption: (key, value) => { model = value } }
})However, I've so far been unable to get the setOption callback to fire (even though we know the event has been emitted by inspecting the wrapper).
There was a problem hiding this comment.
IMO happy to skip component test on this (at least for time being). Also you might be testing implementation details that are likely to change - #2370 (comment)
There was a problem hiding this comment.
I wouldn't like to see a regression where these components move back into the views, so wouldn't expect implementation details to change here as a result of a refactor. Would be thinking more along the lines of the GraphQL form generator, or the Jupyter Lab toolbar plugin (i.e, framework pattern rather than library pattern).
Note, any refactor would be quite distant, so this will stand for a while.
There was a problem hiding this comment.
Can't find a working solution to implementing listeners right now so have removed the test.
* Add functionality to the ViewToolbar component: * Add `action=menu` for dropdowns. * Add `action=input` for text input. * Allow `icon=Object` to configure state-dependent icons. * Reduce icon spacing and improve group divider. * Move the task search & filter controls into the ViewToolbar component. * Switch the Tree and Table views over to the ViewToolbar. * Addresses cylc#471 * Improve the task state filter control: * Add support for task modifiers - closes cylc#1666 * Collapse the control into a single button/menu (take up less space). * Reduce the width of the task search input, automatically increase this width when focused or when text is present in it.
* Ensure the ViewToolbar doesn't scroll out of view. * Closes cylc#991
* These can cause vite to crash.
9028ce4 to
8ede99b
Compare
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
This adds task modifier filtering to the Tree & Table views as well as a bunch of related things:
action=menufor dropdowns.action=inputfor text input.icon=Objectto configure state-dependent icons.this width when focused or when text is present in it.
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.