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

Remove cylc hub button in single user mode #969

Merged
merged 6 commits into from
May 2, 2022

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Mar 22, 2022

The cylc hub button on the dashboard used to be available in single user mode (cylc gui). If clicked this resulted in a 404 page.
This change adds a condition that the gui mode should be multi-user if the button is to display.

I am not sure the preference on the links in the dashboard if the cylc hub button is not visible.

I have stuck with keeping the help links to the right, and the functional links to the left; as currently is the case. I think this looks a little lopsided in single user mode. I’m happy to move the Cylc UI Quickstart to the left list and move the cylc hub to the right for aesthetics.

In single user mode the dashboard now looks as follows:

image
In multi-user mode:
image

These changes close #737
Sibling: cylc/cylc-uiserver#331

  • 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? invisible to users).
  • No documentation update required.

@datamel datamel added the small label Mar 22, 2022
@datamel datamel added this to the 1.2.0 milestone Mar 22, 2022
@datamel datamel self-assigned this Mar 22, 2022
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.

Looks good.

However, how about instead of removing it entirely, display a greyed-out hub icon and You are not running Cylc UI via Cylc Hub or similar. That would hint at additional multi-user capability that's available, and also explain why the Hub button isn't there, in case the lack of it (in comparison to screenshots and documentation, say) is confusing.

@oliver-sanders
Copy link
Member

However, how about instead of removing it entirely, display a greyed-out hub icon

Sounds like a good idea, @datamel there is a prop builtin to the list component we are using which can do this

https://vuetifyjs.com/en/api/v-list-item/#props-disabled

I think it needs this diff:

- v-if=multiUserMode
+ :disabled=multiUserMode

@oliver-sanders
Copy link
Member

oliver-sanders commented Apr 11, 2022

Unit tests are failing due to a rogue .only, a rebase may resolve this, otherwise need to hunt it down:

 error: it.only not permitted (no-only-tests/no-only-tests) at tests/e2e/specs/dashboard.js:47:6:
  45 |       .should('equal', [...WorkflowStateOrder.entries()][0][0])
  46 |   })
> 47 |   it.only('Should disable the cylc hub button with a tooltip in single user mode', () => {
     |      ^
  48 |     cy
  49 |       .visit('/#/')
  50 |       .get('.container > :nth-child(3) > :nth-child(1)')

Functional tests failing?

@datamel
Copy link
Contributor Author

datamel commented Apr 12, 2022

I have made it so a tool tip comes up if you hover over the disabled button for cylc hub.

I am having difficulty testing this, cypress doesn't appear to have a hover function, though is a currently open issue. Instead it is recommend that we use .trigger instead but this does not function for disabled items.

In addition, vue.js renders this cylc-hub-button as an anchor tag
image
; which does not pick up on the be.disabled check of cypress (again recognised in issue on cypress). The recommended work around is to check cy.get(...).should('have.attr', 'disabled') which I can not get to work.
It seems that having a tooltip on a disabled v-list-item is tricky to test. If anyone has ideas then I'm happy to give any suggestions a go.

@@ -129,7 +136,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
</v-list-item-avatar>
<v-list-item-content>
<v-list-item-title class="title font-weight-light">
Cylc UI Quickstart
Cylc UI Quickstart2
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! Changing the page as my reloads were caching even with ctrl F5.

@oliver-sanders
Copy link
Member

I am having difficulty testing this

Perhaps check/scan for the v-list-item--disabled class.

Comment on lines 110 to 126
<template v-slot:activator="{ on }">
<div v-on="on" >
<v-list-item id="cylc-hub-button" :disabled=!multiUserMode :href=hubUrl>
<v-list-item-avatar size="60" style="font-size: 2em;">
<v-icon large>{{ svgPaths.hub }}</v-icon>
</v-list-item-avatar>
<v-list-item-content>
<v-list-item-title class="title font-weight-light">
Cylc Hub
</v-list-item-title>
<v-list-item-title class="title font-weight-light">
Cylc Hub
</v-list-item-title>
<v-list-item-subtitle>
Visit the Hub to manage your running UI Servers
</v-list-item-subtitle>
</v-list-item-content>
</v-list-item>
</div>
</template>
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is off (functionally doesn't matter but proper indentation makes it easier to see the HTML hierarchy)

Copy link
Member

Choose a reason for hiding this comment

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

Indentation better now, but best to avoid putting tags on the same line like this:

<template v-slot:activator="{ on }"> <div v-on="on" >

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MetRonnie I'm not great at these things so have put it through an auto-prettify.

If you are happy then I think we can merge this.

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea why it would want to leave those on the same line. Anyway, it doesn't matter too much

src/views/Dashboard.vue Outdated Show resolved Hide resolved
@hjoliver hjoliver merged commit 463395e into cylc:master May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Cylc Hub button for jupyter-cylc
4 participants