-
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
metadata view: introduce a minimal task metadata view #1886
Conversation
Ping @ScottWales @ColemanTom (you were wanting this for viewing prerequisites etc.) |
Great! thanks for this |
2e5c1b5
to
d6d9e10
Compare
Added offline-data and tests. Many, many enhancements and features left to add to this view, but will leave it at basic metadata, prereqs and outputs for this PR. The main work here is getting the template for the view in place (it's quite different to other views). Further enhancements will slot into this template fairly easily provided the required data is present (and accurate) in the schema. |
d6d9e10
to
cfbc4a9
Compare
v-model="panelExpansionModel" | ||
> | ||
<!-- The metadata --> | ||
<v-expansion-panel class="metadata-panel"> |
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.
IMO most users will be coming here for the pre-reqs and outputs and not the metadata - these should go first and probably not be collapsed.
Could they go in neighbouring Columns?
Happy to spin this out into an issue and not block this PR.
e.g.:
| Prerequisites | | Outputs |
| list of | task icon | |
------------------------------------------------
| Metadata |
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.
Columns aren't an option (or at least aren't always an option) because they won't work in narrow widths.
We can't expand all of the sections by default as there will be a lot of them, from prerequisites, to completion expression satisfaction, to runtime configuration and broadcasts. Prereqs are important, but so are all the other sections.
We could start with nothing expanded, but I thought the title, description and URL (often used to link to documentation) are notably important and deserve to be displayed at all times. Important messages like "do not re-run this task" or "if it fails, try re-running in shortstep mode" will be prominent as a result.
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.
- Manually tested
- Read PR content
I've made some comments, none of which should block the PR going in if they cost any effort to implement.
|
||
<!-- The prereqs --> | ||
<v-expansion-panel class="prerequisites-panel"> | ||
<v-expansion-panel-title color="blue-grey-lighten-2"> |
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.
This one is inconsistent with the others
<v-expansion-panel-title color="blue-grey-lighten-2"> | |
<v-expansion-panel-title color="blue-grey-lighten-1"> |
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.
On purpose.
There will be other sections in the near future. I tried adding a few extras and it ended up as a giant block of "blue-grey" which didn't make it especially obvious that these are individual sections that can be expanded.
I thought it was clearer if the sections were striped ("lighten-1", "lighten-2", "lighten-1", ...).
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.
To me it doesn't really indicate clickability, it looks like it's selected or has keyboard focus or something. Not too important
Unfortunately, this work may be blocked by cylc/cylc-flow#6307 which also imposes some back-compatibility constraints :( I can hack it for |
e7c9e32
to
c9bab0b
Compare
c9bab0b
to
3697fcb
Compare
The cylc-flow blocker to this has not been resolved, however, I have come up with a workaround: cylc/cylc-flow#6403 I have also added an output completion expression (closes #1896) so this should now be more or less a direct analogue of |
3697fcb
to
0dfac1a
Compare
Have you/are you considering a client-side workaround (i.e. in the Other feedback: to me, these look like bullet points rather than yes/no indicators. (Tested on Windows, FF & Chrome) |
It's possible, but it would require iterating over the received delta and the fix would only apply to the info view and not to other views which might want this information (e.g. the tree view currently uses it). I think the long-term fix should be cylc-flow end so would rather stick any bodges / workarounds there if possible. Happy to undraft the hack PR, was hoping we could come up with a more global fix. |
Unblocked by merge of cylc/cylc-flow#6403 ... I think |
isEditable (mutation, authorised) { | ||
return mutation.name !== 'log' && !this.isDisabled(mutation, authorised) | ||
return mutation.name !== 'log' && mutation.name !== 'info' && !this.isDisabled(mutation, authorised) |
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.
isEditable (mutation, authorised) { | |
return mutation.name !== 'log' && !this.isDisabled(mutation, authorised) | |
return mutation.name !== 'log' && mutation.name !== 'info' && !this.isDisabled(mutation, authorised) | |
isEditable (mutation = {}, authorised = false) { | |
return !['log', 'info'].includes(mutation.name) && !this.isDisabled(mutation, authorised) |
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.
Approved although ideally pending the fix for default view selection
* Bare bones task metadata view. * Display task metadata, prerequisites and outputs. * Support for viewing task configuration, broadcasts, etc to follow in future PRs. * Support for markup (e.g. markdown & reStructured text) to arrive in future PRs (pending a decision on how to configure the markup language).
* Added offline data. * Added component tests to cover component display. * Added e2e test to cover opening view, expansion state and data provision.
* Remove the "c" prefix from prerequisite expressions and task aliases. * This matches `cylc show`.
Co-authored-by: Tim Pillinger <[email protected]> Co-authored-by: Ronnie Dutta <[email protected]>
0dfac1a
to
18760a6
Compare
On it. |
(rebased) |
Co-authored-by: Ronnie Dutta <[email protected]>
Re-reviewed and happy. 👍🏼 |
Closes #1071
Closes #1896
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).?.?.x
branch.