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

Tableview #672

Merged
merged 37 commits into from
Nov 24, 2021
Merged

Tableview #672

merged 37 commits into from
Nov 24, 2021

Conversation

giulianoserrao
Copy link
Contributor

@giulianoserrao giulianoserrao commented May 14, 2021

These changes partially address #631 (@hjoliver #631 will stay open after this PR is merged; we will just create more PR's against that issue as we improve the table view, or should we create separate issues?)

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).
  • Appropriate change log entry included.
  • No documentation update required.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2021

Codecov Report

Merging #672 (0261f38) into master (0932f42) will decrease coverage by 0.43%.
The diff coverage is 86.84%.

❗ Current head 0261f38 differs from pull request most recent head c7d9439. Consider uploading reports for the commit c7d9439 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   91.47%   91.03%   -0.44%     
==========================================
  Files          86       91       +5     
  Lines        1701     1841     +140     
  Branches      109      114       +5     
==========================================
+ Hits         1556     1676     +120     
- Misses        119      139      +20     
  Partials       26       26              
Flag Coverage Δ
e2e 78.70% <69.73%> (-1.02%) ⬇️
unittests 79.42% <88.13%> (+1.00%) ⬆️

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

Impacted Files Coverage Δ
src/components/cylc/tree/TreeItem.vue 100.00% <ø> (ø)
src/layouts/Default.vue 100.00% <ø> (ø)
src/store/options.js 100.00% <ø> (ø)
src/views/Workflow.vue 92.85% <ø> (ø)
src/components/cylc/table/callbacks.js 80.00% <80.00%> (ø)
src/services/workflow.service.js 91.45% <81.25%> (-1.69%) ⬇️
src/components/cylc/table/deltas.js 83.07% <83.07%> (ø)
src/components/cylc/table/Table.vue 100.00% <100.00%> (ø)
src/graphql/queries.js 100.00% <100.00%> (ø)
src/store/table.module.js 100.00% <100.00%> (ø)
... and 2 more

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 0932f42...c7d9439. Read the comment docs.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

First quick review 👍

src/layouts/Default.vue Show resolved Hide resolved
src/mixins/tableview.js Outdated Show resolved Hide resolved
src/styles/cylc/_table.scss Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Table.vue Outdated Show resolved Hide resolved
src/views/Table.vue Outdated Show resolved Hide resolved
src/views/Table.vue Outdated Show resolved Hide resolved
src/views/Table.vue Outdated Show resolved Hide resolved
src/mixins/tableview.js Outdated Show resolved Hide resolved
@kinow

This comment has been minimized.

@kinow kinow marked this pull request as draft May 14, 2021 03:37
@datamel
Copy link
Contributor

datamel commented May 14, 2021

@giulianoserrao, my vue is not really up to scratch enough to review, just just to say looks like a substantial piece of work, congrats!

@giulianoserrao
Copy link
Contributor Author

@giulianoserrao, my vue is not really up to scratch enough to review, just just to say looks like a substantial piece of work, congrats!

Thanks @datamel - it's been really challenging, yet fun. I must say, without Bruno and Hilary I would NOT have gone this far! :)

@kinow

This comment has been minimized.

@kinow

This comment has been minimized.

src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Table.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
src/views/Workflow.vue Outdated Show resolved Hide resolved
@giulianoserrao
Copy link
Contributor Author

@giulianoserrao can you apply this diff to get the offline mode working, please?

diff --git a/src/services/mock/workflow.service.mock.js b/src/services/mock/workflow.service.mock.js
index de00c2cc..b4d05500 100644
--- a/src/services/mock/workflow.service.mock.js
+++ b/src/services/mock/workflow.service.mock.js
@@ -217,7 +217,8 @@ class MockWorkflowService extends GQuery {
       data: {
         deltas: {
           added: {
-            workflow: checkpoint.workflows[0]
+            workflow: checkpoint.workflows[0],
+            taskProxies: checkpoint.workflows[0].taskProxies
           }
         },
         workflows: checkpoint.workflows

Done

@kinow
Copy link
Member

kinow commented May 20, 2021

Pushed one commit to fix the Workflow view in the offline mode (mode which e2e tests run in GitHub actions). The e2e tests were failing as the workflow widgets were empty.

@kinow
Copy link
Member

kinow commented May 20, 2021

Rebased fixing conflicts.

@kinow kinow marked this pull request as ready for review May 21, 2021 04:54
kinow and others added 16 commits November 23, 2021 13:45
… fixed bug where subscription had to be restarted to pre-populate table state store, fixed bug where wrong handle was targeted when applying and updating in the table view delta callback handler
… it happens ether way (whether reloaded or not, but it is checked its needed downstream). This fixed an issue where the data wasn't pre-populated until the first emission of the new subscription.
…eeds if we have a valid lookup in existence in the data-store. Reenabled the subscription reload trigger (I will investigate why the two queries are being classes as seperate to ensure this doesn't need to happen for the table view)
…, as they ingest the same data and (currently) differ only in the fragment names
…ion isn't triggered correct because of the way class names are collapsed in production mode
@hjoliver hjoliver force-pushed the tableview branch 6 times, most recently from db55a06 to 0261f38 Compare November 24, 2021 11:04
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.

Had another good play with this, tests now passing. All good 🎉 Thanks @AaronDCole for finishing the branch off (esp.: views driven by shared datastore only). As discussed at last project meeting, merging this as-is; further changes to the tableview can be done as follow-ups.

@hjoliver hjoliver merged commit fad568c into cylc:master Nov 24, 2021
@MetRonnie MetRonnie modified the milestones: 2.0, 1.0 Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants