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.

Sign up for GitHub

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

Jump to bottom

Add slots to the Tree and Tree item components, and then use Tree component in GScan #537

Merged
merged 6 commits into from
Dec 14, 2020
Merged

Add slots to the Tree and Tree item components, and then use Tree component in GScan #537

merged 6 commits into from
Dec 14, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Nov 23, 2020 •
edited
Loading

These changes partially address #502

For the hierarchy in GScan, we don't need/want to re-create a tree/treeitem components pair. Instead, we should be able to use Vue's components slots.

This PR was part of the hierarchy PR for GScan. That PR, however, will take a bit longer to be ready to be merged. And other PR's pending review will also modify the GScan component. So having this simpler PR will make it easier, I think, to merge this change and simplify the hierarchy PR to GScan.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@kinow kinow self-assigned this Nov 23, 2020
@codecov-io
Copy link

codecov-io commented Nov 23, 2020 •
edited
Loading

Codecov Report

Merging #537 (87a1bd9) into master (782dd3f) will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
+ Coverage   80.66%   80.93%   +0.26%     
==========================================
  Files          67       67              
  Lines        1319     1327       +8     
  Branches       80       82       +2     
==========================================
+ Hits         1064     1074      +10     
+ Misses        232      231       -1     
+ Partials       23       22       -1
Flag Coverage Δ
e2e 48.53% <50.00%> (+<0.01%) ⬆️
unittests 78.70% <100.00%> (+0.35%) ⬆️

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

Impacted Files Coverage Δ
src/components/cylc/tree/Tree.vue 62.12% <ø> (ø)
src/components/cylc/tree/TreeItem.vue 96.15% <ø> (ø)
src/components/cylc/gscan/GScan.vue 84.37% <100.00%> (+5.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 782dd3f...87a1bd9. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Nov 23, 2020

Huh, fixing tests took most of the time. Lessons learned:

  • better use wrapper.findAll(TreeItem) instead of relying on wrapped.findAll('.some-css-class')
  • findAll will be replaced by findAllComponents (updated to fix warnings)
  • the easiest way to understand why something is not working in the tests, is using the offline mode to verify what the UI looks like with the given props/data, and/or debug the code

@kinow
Copy link
Member Author

kinow commented Nov 23, 2020

Just needs a couple unit tests to cover the sorting of workflows, then should be ready for review by tomorrow morning 🤞

@kinow kinow mentioned this pull request Nov 23, 2020
Merged
6 tasks
@kinow kinow marked this pull request as ready for review November 23, 2020 20:52
@kinow kinow added this to the 0.3 milestone Nov 23, 2020
oliver-sanders
oliver-sanders approved these changes Nov 24, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Very nice.

@kinow
Copy link
Member Author

kinow commented Nov 25, 2020

Rebased, fixed conflicts.

kinow added a commit to kinow/cylc-ui that referenced this pull request Nov 26, 2020
@kinow
Fix sorting as in cylc#537
7cc65ab
@kinow kinow mentioned this pull request Nov 26, 2020
Merged
8 tasks
@kinow
Copy link
Member Author

kinow commented Dec 8, 2020

Fixed conflicts. Now waiting for CI

kinow
kinow commented Dec 8, 2020
View reviewed changes
src/components/cylc/gscan/GScan.vue
@@ -106,71 +106,62 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
v-if="!isLoading && sortedWorkflows && sortedWorkflows.length > 0"
class="c-gscan-workflows"
>
<div
Copy link
Member Author

Choose a reason for hiding this comment

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

Above this part we have the GScan filter, recently merged ☝️ did some manual testing after fixing conflicts, but thought it was still good to let reviewers know what changed recently

src/components/cylc/gscan/GScan.vue
return -1
}
return 1
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a bug in how sorting is being done. With this I think we should get the right behaviour. Tested with two workflows, altering their states. It should be OK now 🤞

Will need to add more e2e/unit tests for the sorting of GScan I think. Will create an issue for that.

@kinow
Copy link
Member Author

kinow commented Dec 8, 2020

Alright, if CI passes, should be ready for review again.

@kinow kinow mentioned this pull request Dec 8, 2020
Closed
@hjoliver
Copy link
Member

I'll try to get this one reviewed and merged once it is deconflicted.

@kinow
Copy link
Member Author

kinow commented Dec 11, 2020 •
edited
Loading

I'll try to get this one reviewed and merged once it is deconflicted.

Sorry, I hadn't noticed it had conflicts. Will fix these later (woulf be nice if GitHub added a notification for every PR that had new conflicts since my last visit/commit).

@kinow
Copy link
Member Author

kinow commented Dec 13, 2020

Did a bit of testing, appears to be OK. So re-requesting @oliver-sanders ' review, since I had to fix conflicts related to the Cylc Object mutations PR. Hopefully I didn't forget anything 🤞

oliver-sanders
oliver-sanders approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Tested, mutations seem happy.

hjoliver
hjoliver approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hjoliver hjoliver merged commit c9811e3 into cylc:master Dec 14, 2020
@kinow kinow deleted the tree-slots branch January 14, 2021 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@kinow kinow

Labels
None yet
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kinow @codecov-io @hjoliver @oliver-sanders