Skip to content

Combine the two toolbars#2542

Open
ChrisPaulBennett wants to merge 19 commits into
cylc:masterfrom
ChrisPaulBennett:combine-toolbar
Open

Combine the two toolbars#2542
ChrisPaulBennett wants to merge 19 commits into
cylc:masterfrom
ChrisPaulBennett:combine-toolbar

Conversation

@ChrisPaulBennett
Copy link
Copy Markdown
Contributor

@ChrisPaulBennett ChrisPaulBennett commented Apr 30, 2026

This addresses the comments in (#2540) and closes #1455.
It combines both toolbars into one and implements the backburger icon.

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).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@ChrisPaulBennett ChrisPaulBennett self-assigned this Apr 30, 2026
@ChrisPaulBennett ChrisPaulBennett marked this pull request as ready for review April 30, 2026 15:49
@samuel-denton
Copy link
Copy Markdown
Contributor

samuel-denton commented May 1, 2026

The merge of the two toolbar logics looks good to me. It looks to me like one of the 2 was never used but might be wrong about that.

The backburger works I think. I'm not used to it but I think it's obvious what it means. Only thing is that the icon looks like a different font or font weight to the menu burger:
image
Giving the icon a strokeWidth of 1.1 helps them look more similar, although I think I would prefer if both where thiner to be honest:
image
That can be achieved with:

    <v-btn
      icon
      @click.stop="toggleDrawer"
      id="toggle-drawer"
    >
      <v-icon
        :style="drawer
          ? { stroke: 'currentColor', strokeWidth: 1.1 }
          : {}"
      >
        {{ drawer ? icons.backBurger : icons.list }}
      </v-icon>
    </v-btn>

The alternative would be to unify to this set of icons for both:
image

Doing some digging, it looks like there is a further "toolbar" that needs merging (or just removing)!
image
The example @MetRonnie gave where the workflows table now has 2 headings in 2 tool bars is caused by the workflows table view hacking the alert system to add an additional heading. I would guess this was a workaround due to the main toolbar previously being hidden on this page. So the problem code is in: src/views/WorkflowsTable.vue lines 27 to 33 (I'm sure there is a way with GH I could link to that bit of code but I'm not sure how to review code thats not been touched by this PR...)

        <v-alert
          :icon="icons.mdiTable"
          prominent
          color="grey-lighten-3"
        >
          <h3 class="text-h5">{{ $t('Workflows.tableHeader') }}</h3>
        </v-alert>

Just removing that block fixes the duplicate headings / toolbar but does leave the formatting of the workflows table a little off.

Also, some of the page headings on the new toolbar might need some formatting for users E.G.:
image
Should probably be "Workflows Table" rather than "WorkflowsTable".

Similarly the settings page uses an alert for its header, however the text is different so isn't as noticeable.

2 ways I can think to solve this are just to remove the alerts from the pages that use them, or to have the new toolbar display something generic while on these pages, maybe staying as 'dashboard' or maybe just 'cylc' or 'config' or similar. Not sure.

Style wise, for some reason the blue headings doesn't feel quite right to me. It works for workflow headings:
image

But im less of a fan on the workflows table and settings pages, especially after removing the additional heading:
image

@ChrisPaulBennett
Copy link
Copy Markdown
Contributor Author

The mdi library does not have a ForwardBurger icon (which does seem like an oversight on their part) and importing extra icons seems a bit much.
I'm not touching the finer points of the design. Feel free to create your own PR for that. I'm a just trying to combine the toolbars.

Copy link
Copy Markdown
Contributor

@samuel-denton samuel-denton left a comment

Choose a reason for hiding this comment

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

Functionally I'm happy it works.
The settings page still has 2 toolbars/headings but they have different text so arguable if thats ok.
The icons aren't perfectly matched style wise but they work fine.
There are a few small style improvements that could be made but aren't critical, such as text colour on the toolbar on the workflows table page, or a margin being added back to the workflows table page so it matches other pages.

@oliver-sanders
Copy link
Copy Markdown
Member

FYI, we're not actually restricted to MDI icons.

If you can draw it (e.g, on paper) I can SVG'ize it for use in <v-icon /> blocks.

# Conflicts:
#	src/components/cylc/Toolbar.vue
#	src/components/cylc/workspace/Toolbar.vue
@samuel-denton
Copy link
Copy Markdown
Contributor

samuel-denton commented May 7, 2026

I can't identify what changed to cause this, but the toolbar is now not displaying in a few places it should, such as the workflows table, the dashboard and the settings page.
image

Oliver also suggested testing 'standalone views' which I think means replacing 'workspace' in the URL with 'tree' or 'graph'.
These pages do not have a toolbar or menu, but I don't know if that's intended.

@MetRonnie MetRonnie linked an issue May 13, 2026 that may be closed by this pull request
MetRonnie

This comment was marked as resolved.

Comment thread tests/unit/components/cylc/toolbar.vue.spec.js Outdated
Comment on lines 25 to 33
.get('#core-app-bar')
.should('exist')
})
it('Is NOT displayed when looking at the dashboard', () => {
it('Is NOT displayed when using a standalone view', () => {
cy.visit('/#/')
cy
.get('#core-app-bar')
.should('not.exist')
})
Copy link
Copy Markdown
Member

@MetRonnie MetRonnie May 18, 2026

Choose a reason for hiding this comment

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

Testing it is not displayed when looking at the dashboard... immediately followed by testing it is displayed when looking at the dashboard (below).

(The reason this one is passing is probably the page has not fully loaded by the time this assertion is made - which is a gotcha with Cypress.)

Your new test below can replace both of these two above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should still keep it for completeness sake.
But I have amended the test to actually test the thing it's supposed to test.
(Also, what is the proper way to get cypress to wait? The linter doesn't likecy.wait())

Comment thread tests/unit/components/cylc/toolbar.vue.spec.js Outdated
@MetRonnie
Copy link
Copy Markdown
Member

This composable is no longer applicable:

/**
* Composable that returns a computed property for whether we should show
* the hamburger nav drawer button.
*/
export function useNavBtn () {

@MetRonnie MetRonnie added the UX/UI User experience and interface improvements label May 18, 2026
ChrisPaulBennett and others added 2 commits May 19, 2026 10:26
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UX/UI User experience and interface improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Combine the two toolbar components Display drawer expand/collapse button at all times

4 participants