Refactor ViewToolbar component#2472
Conversation
There was a problem hiding this comment.
Extracted from src\components\cylc\ViewToolbar.vue
|
|
||
| /** | ||
| * Scale icon size to button size. | ||
| * https://github.com/vuetifyjs/vuetify/issues/16288 | ||
| * | ||
| * @param {string|number} btnSize - button size | ||
| * @returns {string=} font size | ||
| */ | ||
| export function btnIconFontSize (btnSize) { | ||
| const size = parseInt(btnSize) | ||
| if (Number.isNaN(size)) { | ||
| // do nothing for named sizes ('small', 'large', etc.) | ||
| return undefined | ||
| } | ||
| // Round to even px then convert to rem | ||
| return `${2 * Math.round(0.2 * size) / 16}rem` | ||
| } | ||
|
|
||
| export function btnSizeProps (size) { | ||
| return { | ||
| size, | ||
| style: { | ||
| fontSize: btnIconFontSize(size), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved from src\utils\viewToolbar.js
There was a problem hiding this comment.
At first, each view controlled its own toolbar - fully decentralised.
Then the view-toolbar centralised this to move complexity out of the views and ensure consistency across the app - fully centralised.
This PR moves back to a fully decentralised model where the ViewToolbar is an optional component which doesn't actually do anything and has no meaningful little influence over style.
I understand your itching to re-write this (though it was more work than the rebase!), but it's quite a regression in some ways!
I've suggested a compromise solution with some "soft" centralisation so that the ViewToolbar component actually has some utility to ensure consistency going forward: MetRonnie#13.
Split up this component to make it more maintainable and easier to understand.
MetRonnie
force-pushed
the
view-toolbar
branch
from
0929971 to
1f1df68
Compare
|
The main problem with the current centralised implementation is it creates a new abstraction layer that gets in the way of actually working with components. For example, if you wanted to add a select input to the Table view, you would have to add this in a completely different file ( I've taken some of your suggestions from MetRonnie#13 and added them |
| > | ||
| <v-col cols="8"> | ||
| <ViewToolbar class="my-2"> | ||
| <div class="group" style="flex-basis: 60%;"> |
There was a problem hiding this comment.
Unfortunately while using slots was a nice idea (MetRonnie#13 (comment)), there is no way to pass classes/styles/other attributes through to the parent slot, so the only option is to use a div with the group class
There was a problem hiding this comment.
I don't think there is any legit reason to pass styles or attrs into the group, that's the point.
There was a problem hiding this comment.
I've given the example of controlling the flex basis here?
There was a problem hiding this comment.
I don't think that's a legit example?!
That's what the delegation of style means, components shouldn't be jamming junk like this into the toolbar!
There was a problem hiding this comment.
Components should not be able to control the widths of groups in the toolbar??
There was a problem hiding this comment.
Correct, the toolbar should be in control of the presentation, that's the point of it, otherwise why have one in the first place? Q: Why not just leave every view to sort things out for itself?
E.G, think about toolbars in application development frameworks and how they're used in tools like MS Word, or whatever. The tool (or plugins thereof) bind controls into the toolbar, but the toolbar itself governs how they are displayed, and may provide alternative modes of display (e.g, all text / all icon / text+icon).
Or, to pick a more similar example, here's an example of how to add an item to a toolbar in Jupyter Lab: https://github.com/jupyterlab/extension-examples/blob/31dccfcf2fffb9e199c128744dc20596363932e1/cell-toolbar/src/index.ts#L31-L38
Here, the toolbar is an API (like what we have ATM), you configure your component.
There was a problem hiding this comment.
How else do you propose achieving this ratio of widths that we currently have in the Log view toolbar (which is now using two groups in a ViewToolbar in this PR)? This ratio is specific to the Log view and nowhere else in the app would we need this ratio.
The tool... bind controls into the toolbar, but the toolbar itself governs how they are displayed, and may provide alternative modes of display
Yes and no...
With this PR we still bind controls into the toolbar and the toolbar mainly governs how they are displayed. The difference here is this PR switches from using an intermediate layer of abstraction to using the Vue template syntax directly. A simple change of the defaults in the ViewToolbar component could change, say all buttons to become outlined.
Or, to pick a more similar example, here's an example of how to add an item to a toolbar in Jupyter Lab: https://github.com/jupyterlab/extension-examples/blob/31dccfcf2fffb9e199c128744dc20596363932e1/cell-toolbar/src/index.ts#L31-L38
The goal of this repository is to show how to develop extensions for JupyterLab
But that's a less similar example. We are not defining a developer API for extensibility, we are creating and using our own components.
There was a problem hiding this comment.
Sorry for the belated response, it's taken me too long to get back to this, mb. And, also sorry for the essay of a response...
But that's a less similar example.
We are not defining a developer API for extensibility, we are creating and using our own components.
I think this reveals what it all comes down to. IMO, we are defining a developer API here. The current approach is an API, here you have decided to re-write it as a component library.
It was previously an API for a reason. The whole "view" system is an API and the toolbar a part of it (Cylc's "views" are doing fundamentally the same thing as JupyerLab's "widgets"). For example, views don't open their own connections or manage their own subscriptions, they don't build their own store, they don't store their own options or select their own workflows, they use the APIs we provide centrally and the core infrastructure does all the heavy-lifting. Similarly, they don't [presently] render their own toolbar, they use an API to generate one.
I.E, we are very much "defining a developer API for extensibility", though not necessarily very well at present. Ideally views should be simple units that bind into the cylc-ui API (subscriptions, store, workflowId, toolbar, initialState, etc). They should be things that other people can write and plug in - #1319 (e.g, analysis view, surrogate JupyterLab views, site-specific observability plugins, etc). I appreciate we're a way off of that goal, but we have been gradually moving in this direction for a long time (View Mixins, Workflow Service, Centralised data store, useInitialOptions, ViewToolbar, etc).
I get your preference to the component library approach, it is more flexible obvs, but the API approach seems more appropriate to me and the added flexibility not necessarily a good thing.
My attempt at a compromise was to give in to your component library preference, but ensure the new approach still standardises some "core" features so that we retain centralised control of the toolbar, namely:
- Control name (every control should have a name).
- Tooltips (every control should have a tooltip).
- Groups (every toolbar should use the same system for dividers / groups / toolbar'ey stuff).
- Display of active controls (i.e, highlighting controls in blue).
As you are keen to change the direction of this component, I'm ok giving way here, but would like to ensure the new pattern continues to support these things. My suggestions would be:
- Split the task ID and task state selection components:
- [They were merged in this PR but used to be separate.]
- Although they are often used together, they are fundamentally separate and should not interact.
- Assign a name to each control and enforce this in the Toolbar.
- [Functionality removed in this PR.]
- Automatically add tooltips to each control from the toolbar component.
- [Functionality devolved and enforcement removed in this PR.]
- (consider restoring data-cy back into the toolbar):
- [Functionality devolved and enforcement removed in this PR.]
- I do get why you removed this.
- But coupling to the name of the control does make good sense.
- And the alternative is having two different identifiers for the same thing (which may drift) which is more confusing than helpful IMO.
- (consider a
:colourprop or something like that so the toolbar still calls the shots on the active colour).- [Functionality devolved and enforcement removed in this PR.]
I am sorry for being difficult on this one. To explain why I'm taking this line, cylc-ui was implemented decentralised, I put a lot of time into centralising it (putting in place APIs including this one!), hence I'm personally invested in making sure we don't drift back in the other direction.
There was a problem hiding this comment.
I am not sure why we should clamp down on what is possible for us to implement in a toolbar now, given the goal of making it easy to add simplistic toolbars in possible future extensions that would be developed by other people. It sounds like that should be a separate component with the sort of API you are describing that we could focus on later, whereas the more pressing need is to be able to create/maintain our own toolbars for the components we are developing at the moment without any unnecessary friction.
If you are happy, I would propose leaving the original ViewToolbar component in existence for future use by any such extensions, but moving away from it for the all current views and using simple container divs instead, with the shared styling of the .c-view-toolbar class.
(I am still concerned that the original API may be more burdensome than helpful, even for extensions. To take the Analysis views as an example, the sort of inputs used in its toolbars would be complex to integrate into ViewToolbar in that API)
- Split the task ID and task state selection components:
- [They were merged in this PR but used to be separate.]
- Although they are often used together, they are fundamentally separate and should not interact.
I have recombined them (as they were before) as they are reused by the Tree and Table views, and making them into a component allows easy reuse and avoids duplication. Arguably the task state selection component can be made into its own component too (although it is not used elsewhere yet), though I don't see the problem of still combining this with the ID filter into a reusable component. Note the combined filter state object is used to determine whether to show the empty state notices.
- Assign a name to each control and enforce this in the Toolbar.
- [Functionality removed in this PR.]
Not sure what the utility of this is outside of the idea of the extension API discussed above.
- Automatically add tooltips to each control from the toolbar component.
- [Functionality devolved and enforcement removed in this PR.]
It would be easy enough to add a required tooltip prop to the ViewToolbarBtn component, if really desired. I don't think this is a good idea for other inputs such as text inputs and dropdowns - those have their own labels which are better UX than tooltips in the first place.
(consider restoring data-cy back into the toolbar):
- [Functionality devolved and enforcement removed in this PR.]
- I do get why you removed this.
- But coupling to the name of the control does make good sense.
- And the alternative is having two different identifiers for the same thing (which may drift) which is more confusing than helpful IMO.
As I mentioned in MetRonnie#13 (comment):
[IMO]
data-cyshould be specified explicitly because:
- It won't change unexpectedly and break tests as a result of rewording the title
- It makes it obvious that the element is being tested
If the data-cy attribute drifts from the title/tooltip it can easily be renamed in the source code and the tests, without any trouble of the tests unexpectedly breaking.
There was a problem hiding this comment.
Personally, I think an API approach is more appropriate here, but I get you're keen to change this and am happy to compromise and do it your way. I don't want two ways to define a toolbar, if you're changing it, get rid of the old API. I won't argue against your approach, I just want to maintain a couple of small items of centralised control for maintainability and consistency.
FYI: You've expanded the scope of the toolbar beyond just the toolbar itself into view "controls" (e.g, the log file selector) which are really a separate thing to the toolbar itself and probably a large part of the confusion here. Views are welcome to add components beyond a toolbar as needed, but the toolbar itself aught to be a standard Cylc thing to ensure consistency across the views for the things which can be consistent.
Assign a name to each control and enforce this in the Toolbar.
Not sure what the utility of this is outside of the idea of the extension API discussed above.
To ensure each control in the toolbar has a name for display purposes, e.g, tooltips.
Under the API approach, this is easy to enforce. If switching the approach, please provide a mechanism for this.
Automatically add tooltips to each control from the toolbar component.
It would be easy enough to add a required tooltip prop to the ViewToolbarBtn component, if really desired. I don't think this is a good idea for other inputs such as text inputs and dropdowns - those have their own labels which are better UX than tooltips in the first place.
All controls in a toolbar should have a tooltip which explains their purpose, this is standard UI.
Under the API approach, this was simple, a tooltip was automatically added with the control's configured name. If switching approach, please provide a mechanism which ensures each control has a tooltip. Note, this should work for all inputs, not just buttons.
IMO, the best way is for the toolbar itself to configure these tooltips providing centralised control to ensure consistency of presentation and ease of reconfiguration.
MetRonnie
force-pushed
the
view-toolbar
branch
from
1f1df68 to
c3bb1e8
Compare
3 participants
Split up this component to make it more maintainable and easier to understand.
Follow up to #2370 (comment)
Prerequisite of #2209 (at least, it will make it a lot easier)
Note this does not preclude #1146, it would simple to re-group button props as an array of objects for the relevant settings
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.