Experimental sharing of components between EditorToolbar and DeckOptions

[hgiesel]

This partially implements what I was mentioning in eb3f3dd#commitcomment-49971656.

Rather than inventing the wheel new everytime we need a button or a dropdown, this PR, will puts components shared between EditorToolbar and DeckOptions into sveltelib. This way, we can ensure this design baseline:

The "Save" button is not blue, but that's because I couldn't just pass in btn-primary - the default colors have higher priority.
Also, passing in the entire state into getOptionsDropdown is a bit ugly, this could also possibly refactored, but I haven't looked into in lib.ts that much yet.

But what do you think about this as a proof of concept?

[dae]

The "Save" button is not blue, but that's because I couldn't just pass in btn-primary - the default colors have higher priority.

Will there be a way to bring back some sort of colour?

Also, passing in the entire state into getOptionsDropdown is a bit ugly, this could also possibly refactored, but I haven't looked into in lib.ts that much yet.

The state object holds a bunch of methods that the callbacks need to call, so it might be a bit awkward to pass them all in separately, but if you have an idea for how it could be made clearer, I'm all ears :-)

[dae]

Something seems broken with the typings here - changing 'onClick' to 'onClock' for example elicits no warnings/errors from Typescript during the build.

[hgiesel]

I have a suspicion this has to do with .d.ts files, I'll give it a look.

[dae]

If this is the height of the toolbar, maybe we could rename it to 'height'?

[hgiesel]

It also affects the width of buttons / font-weight, so height seems to be a bit deceiving.

[dae]

The code reuse is a definite improvement, but it's unfortunate that we need to use dynamic components here - I feel like they make the code harder to follow, as we're splitting the UI logic up into a separate svelte and js file, and having to store things in a temporary variable. The dynamic components are necessary to provide an API for add-ons, but for our own usage inside Svelte files, perhaps it would be nicer if we could reference the underlying component classes directly, instead of having to move logic into separate js files?

At the moment ButtonsGroup and DropdownMenu rely on a prop to pass in a list of dynamic components, which makes them difficult to reference directly in a Svelte file. If we changed them to something like this:

DropdownMenu:

<ul {id} class="dropdown-menu">
    <slot />
</ul>

DropdownMenuItem:

<li>[existing html]</li>

Then I think we could change a reactive assignment from

$: menu = dropdownMenu({
                items: [
                    dropdownItem({
                        label: "Add Config",
                        onClick: addConfig,
                    }),
                    dropdownItem({
                        label2: "Rename Config",
                        onClick: renameConfig,
                    }),
                    dropdownItem({
                        label: "Remove Config",
                        onClick: removeConfig,
                    }),
                    dropdownDivider({}),
                    dropdownItem({
                        label: "Save to All Children",
                        onClick: () => save(true),
                    }),
                ],

to something like this in the Svelte body:

<DropdownMenu>
    <DropdownMenuItem label="Add Config" on:click={addConfig} />
    <DropdownMenuItem label="Rename Config" on:click={renameConfig} />
    <DropdownMenuItem label=Remove Config" on:click={removeConfig} />
    <DropdownDivider />
    <DropdownItem label="Save to All Children" on:click={() => save(true)} />
</DropdownMenu>

For ButtonGroup, since we can't iterate over slots, I guess we would need to teach each kind of button how to hide itself instead, which is more verbose, but a bit less magic. And then each button could be passed in like is done with the dropdown menu above.

I feel like that makes the code a fair bit easier to follow. On the other hand, I have not actually implemented this, and there may be something I've missed. What do you think?

Sadly it looks like Svelte doesn't currently provide an API for setting slots from within JS, so if we were to go ahead with this change, to support the JS API as well without code duplication, we'd probably need to make a wrapper component that takes the dynamic components as props, and embeds them in a slot.

[hgiesel]

I also have to agree that slots would certainly feel better, but it also feels like we're settling with a strictly less powerful version of what we can do with dynamic components over better looking syntax. If we were to build up a "GUI component library", should we have two identical versions, one using slots, and one using dynamic components? The two things you already mentioned, and which make me very wary of slots, is that you 1. iterate over slots, and 2. you cannot set them from outside Svelte files. So whenever we'll use slots, it's like we're making it harder for ourselves.

[hgiesel]

I do have some experience with React, so I'm somewhat aware, that a lot of the things I'm arguing for is basically how you'd do it in React, like passing handlers (onClick) directly into the component, or passing components as props, rather than slotting them. Even the way I use on:mount in WithShortcut or WithDropdown is like useRef/forwardRef. It's not like I do it on purpose, it's more like, in striving to have more versatile components, it's just what seems most natural 🤔

[dae]

I'm going to try to finish the rest of the deckoptions translations this evening - if you could avoid changing any other deckconfig files with strings until that's done, that'll avoid conflicts. I'll skip the dropdown buttons for now.

[dae]

They might still need tweaking, but that should minimize conflicts now

[hgiesel]

Right now I'm looking at Bootstrap tabs, which look somewhat like this:

<ul class="nav nav-tabs" id="myTab" role="tablist">
  <li class="nav-item" role="presentation">
    <button class="nav-link active" id="home-tab" data-bs-toggle="tab" data-bs-target="#home" type="button" role="tab" aria-controls="home" aria-selected="true">Home</button>
  </li>
  <li class="nav-item" role="presentation">
    <button class="nav-link" id="profile-tab" data-bs-toggle="tab" data-bs-target="#profile" type="button" role="tab" aria-controls="profile" aria-selected="false">Profile</button>
  </li>
  ...
</ul>

<div class="tab-content">
  <div class="tab-pane active" id="home" role="tabpanel" aria-labelledby="home-tab">...</div>
  <div class="tab-pane" id="profile" role="tabpanel" aria-labelledby="profile-tab">...</div>
  ...
</div>

But putting this into a form that works with slots seems impossible to me. Not only do we have to deal with something iterable, but we have to insert something at two places, once in nav-tabs, and once in tab-content. Everything becomes much easier, once we allow ourselves to pass in the tabs as a iterable data structure, which can be iterated over twice.

// TabWidget.svelte
<script>
   let tabs: TabDefinition[];
</script>

<ul class="nav nav-tabs" id="myTab" role="tablist">
    {#each tabs as tab}
        <NavTab ...>
    {/each}
</ul>

<div class="tab-content">
    {#each tabs as tab}
        <TabPane ...>
    {/each}
</div>

In the end, slots is basically just templates, and it feels like whenever we use templates within a component, we're handicapping them in terms of usability. That's what I'd argue against using slots.

[dae]

From eb3f3dd#commitcomment-50031758

I have the impression that there's a "core" to Svelte that is really awesome, which includes stores, prop passing, and reactivity, but then there's some features which seem less well-thought-out, like slots, which make me think more of templating tools like Jekyll, than it does reactivity libraries. That's not a final judgement of course, Svelte is quite fast-moving imo.

I think that's a fair assessment Henrik; slots do feel a bit half-baked, and it does look like there will be situations where we need to rely on props instead.

Forgive me for dragging this on, but given this is a new path that we'll likely need to live with for years to come, I'm keen to get it right and consider all options :-)

Here's a brief experiment with defining a ButtonGroup2 that takes a slot instead, allowing us to use it directly:

b049290

I kept the dropdown menu items defined as props, but moved the ButtonGroup and DropdownMenu constructors into Svelte, so that OptionsDropdown.svelte and ConfigSelector.svelte don't need to think about dynamic components, and can express the desired display in a natural way. The callbacks are defined inside the Svelte file, and we don't need to pass program state into separate helper JS files. I find it a fair bit easier to follow - not just because of the nicer syntax, but because it's consistent with the way the things flow in the parent components.

If we were to build up a "GUI component library", should we have two identical versions, one using slots, and one using dynamic components?

We definitely don't want to be duplicating logic and styling each time a component has used a slot. One option would be to use a wrapper that converts a list prop into a slot, as can be seen in this prototype:

7e6130a

But this feels a bit like overkill.

If we put aside the need to provide an API for add-ons for a minute, and imagine we were designing a GUI component library just for Anki's internal needs, I wonder if we could avoid some of the JS-based component constructors? I realise we'll need them in some cases to support things like dropdown menus and tabs, but maybe that is more the exception than the common case? Many of our components don't need to contain others programmatically, and for the ones that do, some can be implemented with slots alone. With the current JS-based buttonGroup(), it kind of feels like red/blue async/sync functions - once we use one, any nested component is "tainted" and needs to be invoked the same way.

Do you think the first commit posted above, or something like it, would be a practical approach? Or are you suggesting that we'd be better off avoiding slots altogether, and not just in the cases where they won't work? My instinct is to keep things simple in the cases where we don't currently need the extra complexity, but I'm aware you have more experience in this area, and I don't want to go with such an approach if it will cause big problems for us in the future.

Regarding add-ons, maybe it's still worth considering requiring the user to build their own component with Svelte, like in the add-on? That would give them access to components that take slots as well, and would free us from having to create wrapper functions for every component we wanted to export. And there'd still be the raw HTML escape hatch for users who didn't want to use svelte. But it would mean no type hints/type checking for now (see eb3f3dd#commitcomment-50045348)

On another note, a couple of issues I noticed with this PR:

• with a long deck config name selected, resizing the window to be very narrow cuts off the save button, when I don't think it did before
• the select element in the top bar has an incorrect highlight in night mode
• the hr in the top bar feels a bit off - if we're going to display one, it would be good if its start and end were
  aligned with the start and end of the select and save buttons
• sveltelib is lacking a svelte-check target, and scripts/svelte-check is currently reporting a number of issues

[hgiesel]

Many of our components don't need to contain others programmatically, and for the ones that do, some can be implemented with slots alone. With the current JS-based buttonGroup(), it kind of feels like red/blue async/sync functions - once we use one, any nested component is "tainted" and needs to be invoked the same way.

Yes, that's also what I noticed. That's why I was basically suggesting to go "JS all-the-way", but perhaps I was also judging too soon. As I already mentioned above, maybe I'm trying too hard to make Svelte act like React, and not grokking Svelte along the way.

I think the "layout components", like ButtonGroup, TabWidget, etc. are most important here. Basically every view would start with one of those generic "layout components", and if they use svelte:component, it's all svelte:component from there.

Here's a very interesting example of how somebody implemented a Tab Widget using slots. Maybe if we built on that, and export extension APIs from those layout components (in the example, it's Tabs), we could build components which use more of those idiomatic Svelte features, like slots and on:event, while keeping it extensible.

However I will try this in a different PR, and I'll try it with the editor-toolbar (and ButtonGroup).

[dae]

That does look nice and clean, and I hadn't thought of using setContext in that way. It would mean we miss out on some type checking, as we presumably can't enforce the types of the slot children at compile time, but maybe the ergonomics it brings would make it worth it. Will be interested to see how your trial goes, and I appreciate your willingness to give it a try.

[hgiesel]

Outdated. I'll give this another go.