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

removed the installed state #805

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Oct 11, 2021

PR trio: cylc/cylc-flow#4459, cylc/cylc-uiserver#256, #805

  • The installed state was the same as "stopped" but with the additional
    information that the workflow had not yet been run.
  • Added this information to the workflow status message in cylc-uiserver
    (as it is offline information).
  • Improved the workflow status message provided by cylc-flow.
  • Put the status message into the UI toolbar.
    workflow-status-message

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.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #805 (f1e4a8f) into master (bc7f5ff) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #805   +/-   ##
=======================================
  Coverage   90.96%   90.96%           
=======================================
  Files          84       84           
  Lines        1672     1672           
  Branches      105      106    +1     
=======================================
  Hits         1521     1521           
  Misses        121      121           
  Partials       30       30           
Flag Coverage Δ
e2e 77.81% <100.00%> (ø)
unittests 81.60% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
src/components/cylc/gscan/GScan.vue 90.62% <ø> (ø)
src/components/cylc/gscan/WorkflowIcon.vue 100.00% <ø> (ø)
src/graphql/queries.js 100.00% <ø> (ø)
src/model/WorkflowState.model.js 100.00% <ø> (ø)
src/components/cylc/workflow/Toolbar.vue 78.94% <100.00%> (+1.16%) ⬆️

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 bc7f5ff...f1e4a8f. Read the comment docs.

@oliver-sanders oliver-sanders marked this pull request as ready for review October 11, 2021 12:13
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.

Thanks! Does it supersede #804 too?

span {
color: $font-default-color;
.status-msg {
color: $font-dimished-color;
Copy link
Member

Choose a reason for hiding this comment

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

What is "dimished" meant to be:

  • diminished?
  • dimmed?
  • dimmish?

@hjoliver
Copy link
Member

A couple of issues (which could be considered off-topic for this PR):

  • the status message for "Stopping" is too verbose for the tool bar
  • it doesn't update from Stopping to Stopped (stays Stopping for ever)
  • it's not consistent with the gscan tooltip

shot

@oliver-sanders
Copy link
Member Author

Good catch on the stopping status, will take a look, would like to fix that on this/these PR(s).

Should be able to convert the GScan tooltip to use this summary string too.

* The installed state was the same as "stopped" but with the additional
  information that the workflow had not yet been run.
* Added this information to the workflow status message in cylc-uiserver
  (as it is offline information).
* Improved the workflow status message provided by cylc-flow.
* Put the status message into the UI toolbar.
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Oct 12, 2021

  • [flow] Fixed a bug in the data store where the workflow status wasn't updated to stopping (unless something else triggered an update).
  • [uis] Added logic to change the status from * to stopped on receiving the shutdown message.
  • [ui] Added the status message to the gscan workflow icon tooltips.

opened: cylc/cylc-uiserver#257

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.

Code looks good to me! Will leave final functional review to @hjoliver 👍

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.

Tested, all good now 👍

@hjoliver
Copy link
Member

(For a follow-up: the tool bar icons stay as greyed-out "pause" and "stop" after stopping; the "play" button should reappear).

@hjoliver
Copy link
Member

(Two approvals, but waiting for changes in the sibling PR)

@oliver-sanders oliver-sanders mentioned this pull request Oct 13, 2021
6 tasks
@oliver-sanders
Copy link
Member Author

(For a follow-up: the tool bar icons stay as greyed-out "pause" and "stop" after stopping; the "play" button should reappear).

Play (as in play from stopped) is not implemented in the toolbar (because the mutation didn't exist when the toolbar icons were developed!), should be sorted in #806

@hjoliver
Copy link
Member

Merging now as the sibling PRs are in.

@hjoliver hjoliver merged commit eacb1f0 into cylc:master Oct 15, 2021
@MetRonnie MetRonnie mentioned this pull request Oct 19, 2021
2 tasks
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.

4 participants