-
Notifications
You must be signed in to change notification settings - Fork 27
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
Filter tree item by tasks name and tasks states #483
Conversation
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
==========================================
+ Coverage 61.43% 61.74% +0.30%
==========================================
Files 42 41 -1
Lines 918 941 +23
Branches 42 55 +13
==========================================
+ Hits 564 581 +17
- Misses 341 342 +1
- Partials 13 18 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Aiming at keeping the state of each component instance well separated. First step was to remove the event bus that was shared by instances. Then wrote the simplest approach to filtering I could think of. Not sure if the best, but testing against five, families2, and complex, might tell whether this performs good enough or not. It is performing a simple I used a text field to test the filter, and will add some text boxes for the states. These are not the final UI elements, and are used only for development. Before merging will check with other what it should look like, and translate that into Vuetify and/or custom components 👍 |
Worked OK with Also no problems that I could tell with |
Still need to write unit and functional tests, but before that, I think it'd be good to have more people testing it, so marking it as ready for review 👍 |
This seems to work well, even with two tabs of the complex flow (different filtering in each) 👍 I know the UI aspects are preliminary, so the following comments may be premature or useless!
|
And:
|
Hi @hjoliver !
Definitely not useless. Not sure about premature, but I'm planning to improve the UI elements after I make sure I understand what it must look like with @oliver-sanders .
Also think that'd be useful. Nobody wants to scroll all the way to the top to change the filter, over and over again. +1
That is probably due to the number of elements in the UI, not sure how much related to the filter. When I add a second tree view to the Lumino widget with complex, it also becomes laggy (master/this branch). I'm hoping the improvements for the data store will improve performance for both the Lumino widget and for the filtering.
Initially I was looking for running and succeeded, but it never matched anything. Then I realized I needed to match whatever as in the task states, not in the job. I spent some time confirming, and at least I think the filter is applied correctly against the states of tasks returned in deltas. These may need some adjustments later. Another thing too, is that I'm using a hard-coded list of states, from an
Yup, the first version of the filters (from yesterday 😆 ) had Users could hit enter in the name. But what about the combobox? They would need to use the button. So would we have both, a button and support enter? Or just one option? In the end I went with one option only, the button to filter, to simplify, knowing that this wasn't the final UI. So open for suggestions on how users should trigger the filter 👍 the effort to implement will be the same I guess. Thanks a ton @hjoliver ! |
We are heading toward the definitive list of task and job states as described here: I'm pretty confident that those will stick - i.e. will change very infrequently if ever. Also, if the UI fetched a list of states from the back end and found a new one, it wouldn't know how to display it. |
…nt listeners access
bf517c4
to
1d1d0f9
Compare
What's the interaction? (I've stared at your gif for a bit without seeing it!) |
In the above demo I filter the left-hand tab to show only running tasks, which woks fine, however, as the workflow continues to run new running tasks do not appear in the filtered view (as they would if there were only one tab). When I re-filter the left-hand tab later on it works again. It would appear that the presence of the second tab "freezes" the first? |
Huh, you're right, good spotting. |
…e performance, but we can improve later)
Ops! Good catch!!! It was due to how I defined the reactivity between the workflow tree, and the filters being re-applied. It was configured to react on updates to the tree object, which never (rarely?) changes, as instead we change parts of that object with deltas. Fixed by defining That way Vue should be less busy keeping track of heaps of Observer/Proxy/etc objects, and have more time to handle DOM/VDOM. But that's for later, unless the performance of the filters is not good enough here 👍 |
Ah! And on my computer, in development mode, the memory wasn't that bad, but I could tell there was a slight delay between updates to the filtered items, when comparing with the un-filtered three widget. |
Forgot to mention above, we've also lost the "grey tree blank" (forgot what it's called!) pre-loading graphic, in the tabs. |
(Because switching workflows is broken at the moment I can't test how the filters are effected by changing switching between workflows). |
Fixed in the last commit. Switching between workflows should be working now 🤞 Thanks! |
f585814
to
e691c21
Compare
…tate of filtered=true
Is this ready to go now @kinow ? (You haven't checked the "tests" checkbox in the description, or commented on tests?). |
Writing unit tests today, 50% done, just finishing the unit tests and then adding an e2e test. |
Writing these tests I'm finding a few small issues. @oliver-sanders would it be OK if I asked you to review this one again, probably Monday your time, please? The issues I found are no blockers, but I'd prefer to fix them as I'm writing the unit tests anyway. Example issues (some could have been introduced when I updated code to fix linter issues too):
I'm spending more time with unit tests, and intend to write only one or two functional tests. That's because we are using the simple input and combo box UI elements from Vuetify, and I'm not sure if they match our UI design, nor if they work well with mobile, theming, or if they would have to be changed after we add new Dot/Graph views, etc. So I'm guessing these could change later, so no point in writing a lot of functional tests 👍 (plus, you should be able to switch workflows by then too) |
…ter keydown), only apply filter when button Filter is pressed (create a separate value from the form controls v-models
Most tests written, did some smoke tests, everything seems to be in order. But it's Friday 7PM, so this code comes with no guarantee 😁 will review the code, and probably write a few more tests to increase coverage on Monday NZ time. Then it should be ready for review after that 👍 |
No probs. |
Last commit was to fix CSS styles. I was having a hard time looking at the button with a slightly different height than the other two input (text & select). Had to hard-set it to The right border of the Lumino Dock panel has always been hidden on On mobile, the three elements (text & select & button) get squeezed together. Might need to display one per line. But didn't want to spend too much time working on this UI as it may change later 👍 Just enough to calm down my OCD 😁 |
9731dd7
to
349843b
Compare
Ready for review! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(My usual mainly-functional review on this part of this system) - looks really good, further improvements can be follow-ups IMO. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't delved into the code changes, but functionally it's a pass 🚀
The issue might go away if you delete your |
Tried that I'm afraid, might have a go at debugging again today. |
These changes close #482
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.