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

Svelte Editor Toolbar #1109

Merged
merged 97 commits into from
Apr 16, 2021
Merged

Svelte Editor Toolbar #1109

merged 97 commits into from
Apr 16, 2021

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Apr 1, 2021

First of all, I'm sorry for opening yet another PR.
After the Tag Editor, and potential worries about extensibility, I thought that the Toolbar would be a better target for trying out ways to make Svelte components extensible.

  • The toolbar code resides in /ts/editor-toolbar.
  • There is no longer left / right buttons, but rather I added support for button groups.
  • Not all functionality I implemented, would currently be used by the toolbar, but could be used by add-ons
  • I added the following kinds of buttons:
    • IconButton for buttons which have a big icon
    • CommandIconButton for icon buttons, which are based on document.execCommand (they support the notion of being active)
    • LabelButton for buttons, which have a text label (like "Fields...")
    • SelectButton, which have a <select> menu.
  • I added the following two kinds of menus:
    • DropdownMenu, which is a menu with text items (like the More button currently on AnkiDesktop)
    • ButtonDropdown, which can contain a set of buttons, like the Mathjax button on AnkiMobile currently.

Showing off the toolbar, with two unused features: SelectButton and ButtonDropdown:
Screenshot 2021-04-01 at 19 26 00

  • The buttons can be dynamically adjusted, which would allow for removing the cloze button on non-Cloze note types
  • The toolbar can be turned into a scrollable toolbar, just by changing flex-flow in ButtonGroup. This could also be turned into a parameter for the whole toolbar.
  • The buttons rely on a single input variable for resizing, here is an example of the resized toolbar:

Screenshot 2021-04-01 at 19 15 15

The Toolbar exposes its API in two ways:

  1. editorToolbar is a new global in the editor. It contains the Svelte components which were used to create the buttons, and it also methods updateActiveButtons, and clearActiveButtons, which are exposed via the Module Context of CommandIconButton.svelte.
  2. The toolbar element, accessible with document.getElementById("editorToolbar"), exposes the buttons and menus as Svelte stores, which can be used to update the buttons and menus reactively. This way you can even listen to changes to the toolbar.

Todos (feel free to edit):

  • Remove all unnecessary uses of bridgeCommand. I think only the fields, cards, attachment, and record button will actually still need it. Does that sound right?
  • Turn flex-flow into a input parameter for EditorToolbar, similiar to size.
  • Remove cloze button on non-cloze notetypes
  • Remove the @ts-ignore. Currently Bazel will not recognise methods which were exposed via the Module context (e.g. in CommandIconButton.svelte). (Exchanged with @ts-expect-error)
  • RawButton / passing string as button
  • group identifiers
  • Create individual components for BoldButton etc. (I instead used dynamicComponent which is more a reiteration on withLazyAttributes with better type support)
  • Night mode adjustments
  • The colorpicker is still somewhat ugly / too present
  • Make "depressed" state of buttons look cleaner (more minimal)
  • Linux / Low dpi issue
  • Set forecolor on opening editor
  • Remove the png icons
  • Shortcut API

Unknowns:

  • How much of the code in editor.py can be removed
  • Should shortcuts be dealt with in Qt or in JS? Qt allows for multi-key shortcuts. For JS, I'd probably turn this into a small library (similar to html-filter, which would then be used within editor-toolbar).

Updates:

  • Fixed the picture

Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

My two initial impressions:

  • there's a lot of good functionality here
  • the use of generic Svelte components, stores, and lazy property setting makes the code hard to follow, and loses a lot of type information, and I wonder whether it could be simplified.

For example, withLazyProperties(). The editor toolbar is not instantiated until after i18n has loaded; couldn't we just defer building the components until after that point, like we do with the graphs? By dynamically injecting getters into the components, we're losing the static type checking TypeScript provides us, and the declarations are not as clear.

For the writable stores for the buttons, what goals are we trying to solve? The two I can think of are:

  • buttons that hide and show depending on state
  • the ability for add-ons to modify the available buttons

Are there others I've missed?

Could we solve those goals with a static list that is created after i18n setup instead?

  • for buttons that may not always apply, what if they remained in the list, but just hide or show themselves as appropriate?
  • for add-ons, adding new buttons seems to be the main use case? Are there any add-ons that modify the order or presence of the standard Anki buttons, and is that something we really need to support?

I am wondering if we could skip the whole writable store business in favour of a more standard Svelte declarative approach. Eg, having the toolbar include components like

<FormatButtons />

which in turn lists out the buttons in the standard way:

<CommandIconButton icon={boldIcon} command="bold" tooltip={tr.editingBoldTextCtrlandb()} />
<CommandIconButton icon={italicIcon} command="italic" tooltip={tr.editingBoldTextCtrlandi()} />
...

and at the end of the standard buttons, we can extend the list by iterating over some global that has been modified by add-ons, eg

anki.addons.additionalFormatButtons: (string | SvelteComponent)[]

The string case would be raw HTML, which we can add with {@html}. We could modify Python's _addButton() to transparently add the HTML to that JS list, which would hopefully mean (most) existing add-ons continue to work without changes.

I know your stores-based approach is more flexible, but I feel like it comes at a cost in terms of type checking and ability to reason about the code easily, and I wonder whether it's worth it? What do you think?

@glutanimate are you aware of add-ons where an "append only" approach to the buttons would not work?

Re some of the other points:

resized toolbar:

looks like the same image was attached twice?

Remove all unnecessary uses of bridgeCommand

I think we'd need the HTML editor as well, unless the plan was to show the HTML inline as a toggle like AnkiMobile does.

Remove the @ts-ignore. Currently Bazel will not recognise methods which were exposed via the Module context

I think this is a svelte2tsx issue rather than a Bazel one - SvelteComponent is defined as not having any exports apart from a default one.

ts/editor-toolbar/BUILD.bazel Outdated Show resolved Hide resolved
ts/editor-toolbar/CommandIconButton.svelte Show resolved Hide resolved
@glutanimate
Copy link
Contributor

@dae there are a few add-ons that currently prepend their buttons to the list (so that they're positioned in front of the default buttons), e.g.:

  • MIA / Migaku add-ons
  • Daijirin Dictionary Scraper
  • Semantic HTML tags in editor

I think I also remember writing a couple of add-ons back in the 2.0 days that would position their buttons after particular default buttons, in an attempt to group buttons with similar functionalities together.


Generally speaking:

This looks cool, @hgiesel, both as an extensibility study and in actual use.

A few thoughts purely from a bird's eye add-on dev / user standpoint and no deep dive into the implementation details:

  • I really like the inclusion of ready-made components that add-ons can use to construct their buttons.

    • I do appreciate the freedom that Anki's current approach offers, and I would vote for retaining the ability to define buttons as free-from html, but having an assortment of ready-made button types to choose from is great for speeding up development and for creating a consistent experience for users.

      E.g.: There are a few add-ons that implement select dropdowns right now, but their styling, sizing, etc. all differ in subtle ways, and that can make for pretty inconsistent UX. These changes would fix that.

  • Similarly, I like the approach you've taken with defining button groups. It plays well into the familiarity that a lot of users will have with tools like Word, GDocs, TinyMCE-based editors which work in a similar way.

    • From an add-on dev standpoint, this also opens up the possibility to semantically group add-on buttons together. I think that, to your point @dae, although this is not a use case commonly seen with add-ons right now, I think that it would see quick adoption and significantly improve the current UX:

      Right now, if you install a lot of add-ons that extend the editor, you end up with this huge mish-mash of buttons that all perform completely different actions: Some will format selected text, others will update all fields at once, and yet others will open completely different views.

      If there was a way to separate these, and e.g. place the Mini Format Pack buttons in the formatButtons area, and Image Occlusion in the templateButtons area, that would be pretty neat.

A few thoughts regarding the toolbar API:

  • @hgiesel, would you mind attaching a small MWE on how modifying the toolbar via the store system would currently work for add-ons? Somewhat like this?

    document.getElementById("editorToolbar").buttons.update(updateButtons)
    // where updateButtons takes buttons array, performs changes, and returns it?
  • If so, that feels like a fairly low-level approach that IMHO requires a bit too much boilerplate code to get to the most common use case. Also, I might be mistaken in this, but identifying a particular button group is only possible by index at the moment, right? If so, that could be shaky, especially with add-ons inserting their own button groups.

  • I guess my super general and (implementation-agnostic) wishes for a JS API would be:

    • a simple way to add a button (defined as HTML, or a Svelte component) to a specific button group
    • a simple way to create a new button group, add buttons to it, and add it to the toolbar

    Naively and with no intricate knowledge of Svelte, I would expect these to be accessible via the editorToolbar global, e.g.:

    • editorToolbar.addButton(button, groupIdentifier?), editorToolbar.addButtonGroup(group)
    • or editorToolbar.groups.formatButtons.addButton(), etc.

    But I could also see the stores solution working as a more advanced API, if the object passed to updateButtons was structured more semantically and easier to modify. I.e. following along with the snippet above, being able to add buttons via something like

    function updateButtons(buttons){
      buttons.formatButtons.push(button1);
      buttons.myGroup = [button2, button3, button4];
    }

    Where each button is either a Svelte component or an HTML string (if possible)

    Having global lists that add-ons modify before the toolbar is rendered also seems reasonable, but there would have to be a corresponding hook to ensure that add-ons adding their buttons via JS get the timing right.

    Across all of these approaches, my personal preference would however still be to have some kind of equivalent to Python's addButton on the JS side. IMHO, the easier the API is to use, and the more encapsulated internal state is, the better.

  • On the Python side I'm fully with @dae on preserving _addButton(), and would vote for grouping all buttons added like this together into a separate button group

  • Regarding keyboard shortcuts: I think it could be worthwhile exploring shifting shortcut handling to JS, if only because it would allow add-ons to go for pure JS solutions for their UI if they want to. Some caveats might apply, though, and I'm not sure how well an in-house solution would work vs. Qt's keyboard shortcut system.

@glutanimate
Copy link
Contributor

Also, I know this is a super early iteration, but some quick observations on the design:

  • First of all, I'm a fan! The buttons are clean and feel clicky.

  • One tiny thing I don't like as much is the gradient on the active button state:
    Screenshot_20210403_015318

    To me, it doesn't feel as minimal as the rest of the design and sticks out a bit

  • Also, on my machine, there's a small issue with scrollbars wrapping all button groups:

Screenshot_20210402_230739

Might be Linux-specific or maybe low-DPI-related, haven't really looked into it yet.

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 3, 2021

@dae

I think this is a svelte2tsx issue rather than a Bazel one

I tracked it to here sveltejs/language-tools#550

I think we'd need the HTML editor as well, unless the plan was to show the HTML inline as a toggle like AnkiMobile does.

Yes true. I'd love to tackle the HTML editor in a future PR, but not this one.

With withLazyProperties I wanted to avoid writing the entire module inside of a function, because otherwise it will eagerly evaluate because of static import (In Python it could be avoided because of dynamic imports). If we'd move that code into .svelte that would solve that need, otherwise we could also move the whole code into a function. Both would work.

for buttons that may not always apply, what if they remained in the list, but just hide or show themselves as appropriate?

If we added a show boolean parameter, that would apply display: none, the rounding of corners of "side element" is not applied correctly. See the "Fields" button in the following screenshot:

image

ButtonGroup will construct the ul and li structure. So if the the non-displaying logic would be in there, it would work (even though it's a bit ugly / surprising).

Are there any add-ons that modify the order or presence of the standard Anki buttons, and is that something we really need to support?

The need I saw here wasn't necessarily changing the order or presence of standard Anki buttons, but rather for more freedom over where to place your custom button. Let's say I wanted to add a strikethrough formatting button or a <code> formatting button, I'd want to put them next to the other inline formatting buttons, like italic or bold, ideally just before the eraser, not at the very end.

If we defined a component like so:

// FormattingButtons.svelte
<CommandIconButton icon={boldIcon} command="bold" tooltip={tr.editingBoldTextCtrlandb()} />
<CommandIconButton icon={italicIcon} command="italic" tooltip={tr.editingBoldTextCtrlandi()} />
...

We could not easily insert any new button in between there. I could only replace the whole (or create my own button group, recreating the old one)

One possibility would be to create single files for the individual buttons, let's say

// BoldButton.svelte
<CommandIconButton icon={boldIcon} command="bold" tooltip={tr.editingBoldTextCtrlandb()} />

Obviously that'd necessitate one file for each button. But we'd also be able to skip withLazyProperties. I could give this a try.

The string case would be raw HTML, which we can add with {@html}.

That's a good idea and I'll implement it. I think it would be better suited in a RawButton component though. I'll give it a try.

In the code you'll see that I generally didn't pass SvelteComponent, but rather { component: SvelteComponent, ..args }. These could need some more thorough typing, but they allow the user to call a component just like function, inspired by the Svelte tutorial. These "Component definitions" as object would make a Svelte component a callable entity, which otherwise wouldn't be possible. I think they'd also be great in other places (e.g. GraphsPage).

@glutanimate

Thanks for your suggestions, all of them sound good, especially group identifiers.

how [would] modifying the toolbar via the store system would currently work for add-ons? Somewhat like this?

Absolutely correct.

@dae
Copy link
Member

dae commented Apr 3, 2021

In the code you'll see that I generally didn't pass SvelteComponent, but rather { component: SvelteComponent, ..args }. These could need some more thorough typing, but they allow the user to call a component just like function, inspired by the Svelte tutorial. These "Component definitions" as object would make a Svelte component a callable entity, which otherwise wouldn't be possible.

Thanks for elaborating and for the link, that helps me understand the issue you're trying to solve a bit better.

Since we're effectively losing the typechecking when constructing components dynamically like this, how about we a) define a helper function to construct each type of component with the proper arguments, and b) turn the string properties into callbacks? Eg

diff --git a/ts/editor-toolbar/CommandIconButton.svelte b/ts/editor-toolbar/CommandIconButton.svelte
index ed45d23cc..d41bce0ee 100644
--- a/ts/editor-toolbar/CommandIconButton.svelte
+++ b/ts/editor-toolbar/CommandIconButton.svelte
@@ -39,7 +39,7 @@
     export let id = "";
     export let className = "";
     export let props: Record<string, string> = {};
-    export let title: string;
+    export let tooltip: () => string;

     export let icon = "";
     export let command: string;
@@ -61,6 +61,6 @@
     }
 </script>

-<SquareButton {id} {className} {props} {title} {active} {onClick} on:mount>
+<SquareButton {id} {className} {props} title={tooltip()} {active} {onClick} on:mount>
     {@html icon}
 </SquareButton>

and then something like

import type { SvelteComponentDev } from "svelte/internal";

interface CommandIconArgs {
    icon: string;
    bridgeCommand: string;
    tooltip: () => string;
}

interface DynamicSvelteComponent {
    component: typeof SvelteComponentDev;
}

type DynamicCommandIcon = CommandIconArgs & DynamicSvelteComponent;

function commandIconButton(args: CommandIconArgs): DynamicCommandIcon {
    return {
        component: CommandIconButton,
        ...args,
    };
}

const boldButton = commandIconButton({
    icon: boldIcon,
    bridgeCommand: "bold",
    tooltip: tr.editingBoldTextCtrlandb,
});

const italicButton = commandIconButton({
    icon: italicIcon,
    bridgeCommand: "italic",
    tooltip: tr.editingItalicTextCtrlandi,
});

I noticed let props: Record<string, string> in the code above. This is difficult to follow; it would be much nicer if any extra arguments it can take are properly encoded in the type system, so that we get code completion, and can follow along with the code more easily. If I hover over the writing buttons store definition, I currently see:

(property) EditorToolbar.buttons: Writable<(DynamicCommandIcon | Record<string, unknown>)[][]>

The latter part doesn't provide me with any information about what I can put in it, and I think maintenance will be a lot easier if it's clear exactly what needs to be placed in it.

(As an aside, in ButtonGroup.svelte you define export type Buttons = ButtonDefinition | Buttons[];, but then the {#each} clause seems to iterate over it unconditionally - maybe the typing or the code needs adjusting?)

A well-typed writable store that avoids any generic dictionaries would largely alleviate my concerns there. But coming back to the other point for the sake of argument:

We could not easily insert any new button in between there. I could only replace the whole (or create my own button group, recreating the old one)

My thinking was to have each section have its own global. So add-ons wouldn't be able to insert their icon before a standard one in a particular group, but they would be able to append to the group that made most sense (eg anki.addons.additionalTemplateButtons, etc). I'm not strongly pushing for this approach, just think it is worth considering, as it would enable us to bypass the whole writable stores thing. I gather from your comments @glutanimate that targeting the relevant section seems to be the important part?

ButtonGroup will construct the ul and li structure. So if the the non-displaying logic would be in there, it would work (even though it's a bit ugly / surprising).

Yep, it could just use a derived variable that filters out any disabled components

Should shortcuts be dealt with in Qt or in JS? Qt allows for multi-key shortcuts. For JS, I'd probably turn this into a small library (similar to html-filter, which would then be used within editor-toolbar).

For the multi-key shortcuts, I just realised that we're using cmd+m for MathJax on Macs, which conflicts with the standard minimize shortcut - Qt seems to override the default. If we try to capture it in JS, does that work, or does the window get minimized instead?

Assuming we can capture it, what if the multi-shortcut keys brought up the relevant menu, eg cmd+m → shows mathjax dropdown, with each option showing the shortcut key next to it, and a subsequent keystroke activates the menu. Not sure how tricky that would be to implement though :-)

@glutanimate
Copy link
Contributor

glutanimate commented Apr 3, 2021

I gather from your comments @glutanimate that targeting the relevant section seems to be the important part?

I'd say so, yeah, but I would also very much love the ability to create new sections/groups for add-ons that add multiple buttons to the editor.

But any additional configurability, i.e. in terms of the position within sections, would of course also be much appreciated and contribute towards the overall user experiences we can create. E.g. similar to what Henrik said, if my add-on adds a button that relates to audio recording, or working with clozes, as a user I would expect it to be positioned next to the buttons in question.

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 8, 2021

@dae
I've rewritten the logic for dynamic components, which is now based on sveltelib/dynamicComponent.

There are a few gotchas:

  • I wanted to write the ComponentProps types inside of x.svelte, however that didn't work, because all exported objects from the module context loose their type information, which is why I put them into x.d.ts files.
  • I tried to minimise the amount of boilerplate, which is why there's some TS magic in sveltelib/dynamicComponent. One frustrating fact is that exported Svelte components, do not type the props themselves, which explains the need for the .d.ts files. The default value for the Props type has as its base the untyped props type (Record<string, unknown>) that is defined on the svelte component: NonNullable<ConstructorParameters<Comp>[0]["props"]>. If our tooling started to type this, we could just start to use this type directly instead of Props and could remove the .d.ts files.
  • I left in the logic for setting lazy properties, which is now typed however. I couldn't bring myself to delete it just yet, because I don't like how the upstream i18n implementation would impact the logic of downstream components. If you still dislike it, I'll remove it.

Copy link
Contributor Author

@hgiesel hgiesel left a comment

Choose a reason for hiding this comment

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

@glutanimate If you don't mind, could you check whether that fixes the scrollbars appearing on Linux?

Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Looks like it's coming together!

I wanted to write the ComponentProps types inside of x.svelte, however that didn't work, because all exported objects from the module context loose their type information, which is why I put them into x.d.ts files.

I tried to minimise the amount of boilerplate, which is why there's some TS magic in sveltelib/dynamicComponent. One frustrating fact is that exported Svelte components, do not type the props themselves, which explains the need for the .d.ts files. The default value for the Props type has as its base the untyped props type (Record<string, unknown>) that is defined on the svelte component: NonNullable<ConstructorParameters[0]["props"]>. If our tooling started to type this, we could just start to use this type directly instead of Props and could remove the .d.ts files.

Yeah, both of these are unfortunate. I'm not a huge fan of React, but I do look upon its first-class TypeScript support with envy.

Since we are already using a custom rule to build the Svelte, we could potentially take matters into our own hands at one point and modify the generated .d.ts files to our needs, but I do not know how hard it would end up being, and I'm hoping if we wait a while, Svelte's tooling will improve.

I left in the logic for setting lazy properties, which is now typed however. I couldn't bring myself to delete it just yet, because I don't like how the upstream i18n implementation would impact the logic of downstream components. If you still dislike it, I'll remove it.

The fact that types are preserved now is definitely an improvement, but I can't help but feel that the solution is worse than the problem here. When you speak of "downstream components", am I understanding correctly that you're concerned add-on authors will need to write:

const customButton = commandIconButton({
        icon: "custom.png,
        command: "custom",
        tooltip: () => "custom tooltip",
);

Instead of the following?

const customButton = commandIconButton({
        icon: "custom.png,
        command: "custom",
        tooltip: "custom tooltip",
);

In exchange for saving those 6 characters, every button definition in Anki's codebase goes from

const boldButton = commandIconButton({
    icon: boldIcon,
    command: "bold",
    tooltip: tr.editingBoldTextCtrlandb,
);

to

const boldButton = commandIconButton<CommandIconButtonProps, "tooltip">(
    {
        icon: boldIcon,
        command: "bold",
    },
    {
        tooltip: tr.editingBoldTextCtrlandb,
    }
);

and to support this, we need this extra helper that uses dynamic property injection and advanced TS typing features:

export interface DynamicSvelteComponent<
    T extends typeof SvelteComponentDev = typeof SvelteComponentDev
> {
    component: T;
}

export const dynamicComponent = <Comp extends typeof SvelteComponentDev>(
    component: Comp
) => <
    Props extends NonNullable<ConstructorParameters<Comp>[0]["props"]>,
    Lazy extends string = never
>(
    props: Omit<Props, Lazy>,
    lazyProps: {
        [Property in keyof Pick<Props, Lazy>]: () => Pick<Props, Lazy>[Property];
    }
): DynamicSvelteComponent<Comp> & Props => {
    const dynamicComponent = { component, ...props };

    for (const property in lazyProps) {
        const get = lazyProps[property];
        const propertyDescriptor: TypedPropertyDescriptor<
            Pick<Props, Lazy>[Extract<keyof Pick<Props, Lazy>, string>]
        > = { get, enumerable: true };
        Object.defineProperty(dynamicComponent, property, propertyDescriptor);
    }

    return dynamicComponent as DynamicSvelteComponent<Comp> & Props;
};

The logic there may seem obvious having just written it, but I fear it will be a time sink in the future when we return to this part of the code and try to follow the code flow. It feels like what we're losing in terms of code clarity and maintainability isn't worth the minor inconvenience to add-on authors?

I don't have a strong preference for individual callbacks, and I think there are other ways this could be solved as well that are less magical. We could have a dynamic component be an interface of { component, propBuilder: () → Props }, or get a bit more OOPy and turn it into a factory class with a .build() method that returns an instance of the component. Not suggesting that's a better option than individual callbacks, just throwing out ideas :-)

ts/editor-toolbar/ColorPicker.svelte Outdated Show resolved Hide resolved
@hgiesel hgiesel marked this pull request as draft April 9, 2021 11:10
@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 9, 2021

In exchange for saving those 6 characters, every button definition in Anki's codebase goes from [...]

What I'm worried here is that we're guessing which properties might be provided by external APIs in the future. So right now label and tooltip need to be provided by the i18n API. I remember reading an article about Wikipedia where they implemented a different set of icons depending on whether the Wiki language was LTR or RTL, so should we wrap icon into a callback, just to be sure for the future?

dynamicComponent is hard to follow already now, even though I've written it :)
A big issue is that Typescript does not type the DOM APIs that well, e.g. Object.defineProperty is typed as:

(o: any, p: string | number | symbol, attributes: PropertyDescriptor & ThisType<any>): any

I will think about this a bit more and the alternative implementations you suggested...

EDIT:

What do you think about mimicing the old i18n API, and wrapping the individual button groups into function, taking tr as a parameter:

const labelButton = dynamicComponent(LabelButton);
const buttonGroup = dynamicComponent(ButtonGroup);

export function getNotetypeButtons(tr: I18n) {
    const fieldsButton = labelButton<LabelButtonProps>({
            onClick: () => bridgeCommand("fields"),
            disables: false,
            label: `${tr.editingFields()}...`,
            tooltip: tr.editingCustomizeFields(),
    });

    const cardsButton = labelButton<LabelButtonProps>({
            onClick: () => bridgeCommand("cards"),
            disables: false,
            label: `${tr.editingCards()}...`,
            tooltip: tr.editingCustomizeCardTemplatesCtrlandl(),
    });

    return buttonGroup<ButtonGroupProps>({
            id: "notetype",
            buttons: [fieldsButton, cardsButton],
    });
}

I assume when you changed the i18n API, it was so we can be more selective about which strings to load?

@dae
Copy link
Member

dae commented Apr 9, 2021

What I'm worried here is that we're guessing which properties might be provided by external APIs in the future. So right now label and tooltip need to be provided by the i18n API. I remember reading an article about Wikipedia where they implemented a different set of icons depending on whether the Wiki language was LTR or RTL, so should we wrap icon into a callback, just to be sure for the future?

Yep, that's a good point.

Tree shaking was the motivation for moving to free-standing functions - if they're not at the top level, tools like esbuild don't know if they'll be used or not, and we end up including 1000+ rather verbose functions into the output JS.

In your example, isn't passing tr around unnecessary? I would have thought simply placing the button definitions inside another function would be sufficient, since in either case, the i18n data should be available by the time the outer function is called.

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 9, 2021

In your example, isn't passing tr around unnecessary?

Ah, yes of course :)

Okay, I see. In that case I'll move forward with the functions solution.

@dae
Copy link
Member

dae commented Apr 9, 2021

Sounds good :-)

One final comment about the example above; if we can eliminate the need for the lazy properties, maybe dynamicComponent could take the props type up front, so we don't have to pass them in to each labelButton() / buttonGroup() call?

@dae
Copy link
Member

dae commented Apr 10, 2021

Thanks for making those changes Henrik, it feels much easier to follow now :-)

@hgiesel hgiesel force-pushed the toolbar branch 2 times, most recently from 3f21864 to 9222df1 Compare April 14, 2021 12:37
@import "ts/node_modules/bootstrap/scss/functions";
@import "ts/node_modules/bootstrap/scss/variables";
@import "ts/node_modules/bootstrap/scss/mixins";
@import "ts/node_modules/bootstrap/scss/dropdown";
Copy link
Member

Choose a reason for hiding this comment

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

Most of the ts/svelte code I pushed in 7f738c1 is just me fumbling my way through and is probably not best practice :-) But one thing you may want to adopt is the way I've added a ts/bootstrap folder that exposes a sass library - that way you can add it as a dependency and import from it cleanly:

7f738c1#diff-3dfaaa14709dfa6fec287bccf73739804b971790e23d6b24dea8661c7bacd6d2R14
7f738c1#diff-eeb95d9e3e56521ca955e5a954d0d63e0063981a63bdf62404b130151e6860ebR4

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

@dae
The resolution to external scss files works good for .scss files, however for SCSS @import/@use in .svelte files, I can get either get them to either compile, or pass svelte_check.
To make them build, I used ts/node_modules/bootstrap/scss/{functions,variables}

I had a look at the output directory, and tried the following, which passes the //ts/editor-toolbar:svelte_check, but it doesn't build.

@import "../../../../ts/bootstrap/functions";
@import "../../../../ts/bootstrap/variables";

@dae
Copy link
Member

dae commented Apr 15, 2021

When we compile with sass_binary(), it takes care of declaring all the inputs so they trigger a recompile if necessary, and it tells sass the folders it should look for files:

dae@dip:dtop/ts/deckconfig% bazel aquery deckconfig-base_sass
INFO: Invocation ID: 4efd5abf-ed1a-4038-aa68-38571a811810
INFO: Analyzed target //ts/deckconfig:deckconfig-base_sass (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
action 'SassCompiler ts/deckconfig/deckconfig-base.css'
  Mnemonic: SassCompiler
  Target: //ts/deckconfig:deckconfig-base_sass
  Configuration: darwin-fastbuild
  ActionKey: c3f5cb8074844b6975bb4cf00958bbbacd0354195d8c9040cf0409cb98060767
  Inputs: [bazel-out/darwin-fastbuild/bin/ts/bootstrap/_accordion.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_alert.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_badge.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_breadcrumb.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_button-group.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_buttons.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_card.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_carousel.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_close.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_containers.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_dropdown.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_forms.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_functions.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_grid.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_helpers.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_images.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_list-group.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_mixins.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_modal.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_nav.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_navbar.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_offcanvas.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_pagination.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_popover.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_progress.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_reboot.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_root.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_spinners.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_tables.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_toasts.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_tooltip.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_transitions.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_type.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_utilities.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/_variables.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/bootstrap-grid.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/bootstrap-reboot.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/bootstrap-utilities.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/bootstrap.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_floating-labels.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_form-check.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_form-control.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_form-range.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_form-select.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_form-text.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_input-group.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_labels.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/forms/_validation.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_clearfix.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_colored-links.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_position.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_ratio.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_stretched-link.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_text-truncation.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/helpers/_visually-hidden.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_alert.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_border-radius.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_box-shadow.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_breakpoints.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_buttons.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_caret.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_clearfix.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_container.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_deprecate.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_forms.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_gradients.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_grid.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_image.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_list-group.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_lists.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_pagination.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_reset-text.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_resize.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_table-variants.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_text-truncate.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_transition.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_utilities.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/mixins/_visually-hidden.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/utilities/_api.scss, bazel-out/darwin-fastbuild/bin/ts/bootstrap/vendor/_rfs.scss, bazel-out/host/bin/external/io_bazel_rules_sass/sass/sass.sh, bazel-out/host/bin/external/io_bazel_rules_sass/sass/sass_loader.js, bazel-out/host/bin/external/io_bazel_rules_sass/sass/sass_require_patch.js, bazel-out/host/internal/_middlemen/external_Sio_Ubazel_Urules_Usass_Ssass_Ssass.sh-runfiles, ts/deckconfig/deckconfig-base.scss, ts/sass/_vars.scss, ts/sass/base.scss, ts/sass/scrollbar.scss]
  Outputs: [bazel-out/darwin-fastbuild/bin/ts/deckconfig/deckconfig-base.css]
  ExecutionInfo: {supports-workers: 1}
  Command Line: (exec bazel-out/host/bin/external/io_bazel_rules_sass/sass/sass.sh \
    --no-error-css \
    '--style=compressed' \
    --no-source-map \
    '--load-path=./' \
    '--load-path=bazel-out/darwin-fastbuild/bin/' \
    '--load-path=bazel-out/darwin-fastbuild/bin/' \
    ts/deckconfig/deckconfig-base.scss \
    bazel-out/darwin-fastbuild/bin/ts/deckconfig/deckconfig-base.css)
  ExecutionInfo: {supports-workers: 1}

When we build svelte files, we're indirectly calling the sass compiler via svelte-preprocess, and we're not declaring the sass files we depend on, which could lead to a flaky build. I think we'll need to update the svelte rule to pass these in - I will look into it.

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

A short update on my progress here. I am somewhat intent on merging this PR soon, and not let it sit and grow much longer.

  • I set as a task "Set forecolor on opening editor". Originally I wanted to use this to add a Backend endpoint {G,S}etEditorPreferences, however I'm not so sure I still want to implement this. I think almost all of the editor preferences would make more sense on a per notetype basis, including lastColour. Maybe this could be tackled in a follow-up PR. What do you think?
  • I don't have a Linux system at hand to check the low DPI issue glutanimate pointed at.
  • I think removing old icons / code can also easily be done in follow-up PRs.
  • The Preview button in the browser does not show its active state upon clicking. If possible, I'd prefer to tackle a shortcut API for the webviews with a functionality of shortcuts activating the click handler, in another PR.
  • I did not add any Latex buttons / menu items, however the shortcuts are still functional. From your interactions on the forum, and missing support on AnkiMobile, I was under the impression, that you were interested in deprecating it?

@dae
Copy link
Member

dae commented Apr 15, 2021

Guh, so this turns out to be a big pain in the neck. The standard way of specifying preprocess options to svelte-check is a svelte.config.js file, which doesn't seem to work when running under Bazel. Luckily sass will also take paths in an env var, so that unblocks us.

Here's an example of how you can use the latest code:

diff --git a/ts/deckconfig/BUILD.bazel b/ts/deckconfig/BUILD.bazel
index b53e6d0a6..0bfa900a6 100644
--- a/ts/deckconfig/BUILD.bazel
+++ b/ts/deckconfig/BUILD.bazel
@@ -24,6 +24,9 @@ svelte_names = [f.replace(".svelte", "") for f in svelte_files]
 compile_svelte(
     name = "svelte",
     srcs = svelte_files,
+    deps = [
+        "//ts/bootstrap:scss",
+    ],
 )
 
 copy_bootstrap_icons(
@@ -112,5 +115,5 @@ svelte_check(
     srcs = glob([
         "*.ts",
         "*.svelte",
-    ]),
+    ]) + ["//ts/bootstrap:scss"],
 )
diff --git a/ts/deckconfig/DeckConfigPage.svelte b/ts/deckconfig/DeckConfigPage.svelte
index a6ae44078..2ac04e93d 100644
--- a/ts/deckconfig/DeckConfigPage.svelte
+++ b/ts/deckconfig/DeckConfigPage.svelte
@@ -23,7 +23,8 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     let defaults = state.defaults;
 </script>
 
-<style>
+<style lang="scss">
+    @import "ts/bootstrap/mixins";
     .outer {
         display: flex;
         justify-content: center;

@dae
Copy link
Member

dae commented Apr 15, 2021

Merging sooner rather than later is definitely good for avoiding merge conflicts and duplicated effort. To make that easier in the future, I'd recommend trying to keep changes (mostly) isolated to start with - putting them behind a feature flag, or just not hooking them up. Provided the tests pass, it's much easier to merge such code early, as there's no risk of breaking things for existing users - while using git HEAD as a daily driver is not the best idea, some people do do that. I do realise this PR involved a lot of experimental changes and refactoring though, making it not as practical in this case.

I set as a task "Set forecolor on opening editor". Originally I wanted to use this to add a Backend endpoint {G,S}etEditorPreferences, however I'm not so sure I still want to implement this. I think almost all of the editor preferences would make more sense on a per notetype basis, including lastColour. Maybe this could be tackled in a follow-up PR. What do you think?

I don't recall anyone ever asking for separate colours per note type, so I wonder if the demand is there? As an example of why this might not be a clear-cut decision, AnkiMobile stores the "scratchpad active" state separately for each card template. There have been cases where users thought the setting was being forgotten because it differed as they went through their cards, and they expected it to be a global on/off.

I don't have a Linux system at hand to check the low DPI issue glutanimate pointed at.

@glutanimate can you confirm the scrollbars went away?

I think removing old icons / code can also easily be done in follow-up PRs.

yep

The Preview button in the browser does not show its active state upon clicking. If possible, I'd prefer to tackle a shortcut API for the webviews with a functionality of shortcuts activating the click handler, in another PR.

sounds good. One related thing that will need fixing at one point, but is not urgent: the tooltip for the MathJax buttons shows Ctrl+m on a Mac; we need to replace Ctrl with Command, doing something like aqt/utils.py:shortcut()

I did not add any Latex buttons / menu items, however the shortcuts are still functional. From your interactions on the forum, and missing support on AnkiMobile, I was under the impression, that you were interested in deprecating it?

While I think MathJax is a better solution for most users, it doesn't fit everyone's use case, so I think we probably still need to show those options in the computer version.

Some other points:

Screen Shot 2021-04-15 at 3 02 06 pm

  • matching the colour of the Qt buttons would make things look more polished. The difference is more subtle in day mode, but still there:

Screen Shot 2021-04-15 at 3 07 41 pm

  • if adding bullets/numbers is a trivial change, that might be worth slipping in, as when changing up the interface it's nice to give users something they didn't have before

@dae
Copy link
Member

dae commented Apr 15, 2021

(I don't see any scrollbars on a Linux machine here, but did not check on Linux with the earlier commits)

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

However, I still cannot seem to pass svelte_check with SCSS imports. I put in what you suggested above, using //ts/sass/bootstrap instead in f9565cb. Have I overseen something obvious?

*  Format shortcuts in monospace font and increase padding a little bit
@dae
Copy link
Member

dae commented Apr 15, 2021

You need to pass the sass as a dep for both the Svelte compile and the Svelte check. Sorry for the noisy diff, I've got VSCode set up to auto-format.

diff --git a/ts/editor-toolbar/BUILD.bazel b/ts/editor-toolbar/BUILD.bazel
index 5576245f3..acf06ef08 100644
--- a/ts/editor-toolbar/BUILD.bazel
+++ b/ts/editor-toolbar/BUILD.bazel
@@ -14,22 +14,23 @@ compile_svelte(
     name = "svelte",
     srcs = svelte_files,
     deps = [
+        "//ts/sass:button_mixins_lib",
         "//ts/sass/bootstrap",
     ],
 )
 
 compile_sass(
-    group = "local_css",
     srcs = [
+        "bootstrap.scss",
         "color.scss",
         "legacy.scss",
-        "bootstrap.scss",
     ],
+    group = "local_css",
+    visibility = ["//visibility:public"],
     deps = [
-        "//ts/sass/bootstrap",
         "//ts/sass:button_mixins_lib",
+        "//ts/sass/bootstrap",
     ],
-    visibility = ["//visibility:public"],
 )
 
 ts_library(
@@ -52,14 +53,14 @@ ts_library(
         exclude = ["index.ts"],
     ),
     deps = [
+        "//ts:image_module_support",
         "//ts/lib",
         "//ts/lib:backend_proto",
         "//ts/sveltelib",
-        "//ts:image_module_support",
-        "@npm//svelte",
-        "@npm//bootstrap",
         "@npm//@popperjs/core",
         "@npm//@types/bootstrap",
+        "@npm//bootstrap",
+        "@npm//svelte",
     ],
 )
 
@@ -140,6 +141,8 @@ svelte_check(
     srcs = glob([
         "*.ts",
         "*.svelte",
-    ]) + ["//ts/sass/bootstrap"],
+    ]) + [
+        "//ts/sass/bootstrap",
+        "//ts/sass:button_mixins_lib",
+    ],
 )
-

The other errors look to be caused by referencing ts/bootstrap instead of ts/sass/bootstrap.

I actually wrote that add-on ;-) I've always thought people are better off binding shortcut keys of their liking than clicking with the mouse, but do realise some people prefer to use the mouse. AnkIMobile uses a small palette of choices as well, and I have no objections to such an implementation on the desktop version, and it'll likely be easier to do in HTML than it was with Qt.

@dae
Copy link
Member

dae commented Apr 15, 2021

(note how the bootstrap is a single dep listing to get access to any of the partials - we could do the same thing with the various ts/sass/*.scss files, building them into a single sass_library instead of separate ones for each file)

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

(note how the bootstrap is a single dep listing to get access to any of the partials - we could do the same thing with the various ts/sass/*.scss files, building them into a single sass_library instead of separate ones for each file)

I guess that road cleaner, the SASS compilation is very quick, and before we have 10+ _lib sass dependencies...

@dae
Copy link
Member

dae commented Apr 15, 2021

The library doesn't do any compilation, it's just a way of bundling up a bunch of inputs that downstream code may or may not want to use. So putting 10 files in a library doesn't mean they'll all be compiled, unless you import all of them.

@dae
Copy link
Member

dae commented Apr 15, 2021

(there is a small cost to create a symlink to each one when building a target (more so on Windows), but I suspect it won't be a big issue)

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

So now I get the issue with Bootstrap Javascript:

Cannot find module 'bootstrap/js/dist/dropdown' or its corresponding type declarations. (ts)

I do not get this locally when running svelte_check, which is even more confusing.


So here's a compilation of things I'd want to tackle in upcoming PRs, in rough order of priority:

  1. UPDATED: Move special AddCards dialog buttons to the new toolbar as well. I think this warrants a new kind of FullWidthButtonGroup
  2. List formatting options (this will be probably very quick and easy, but I'd still prefer it not to do in this PR)
  3. Paragraph formatting options (remember that thread on the forum with a user wanting back the old <div> behavior rather than <br>?)
  4. Advanced Color Picker
  5. Shortcut API
  6. Bonus: Edit HTML in-place

In this PR, at this point I only want to tackle the color clashes between Bootstrap and Qt, you mentioned above.

@dae
Copy link
Member

dae commented Apr 15, 2021

@@ -144,6 +144,6 @@ svelte_check(
     ]) + [
         "//ts/sass:button_mixins_lib",
         "//ts/sass/bootstrap",
+        "@npm//@types/bootstrap",
     ],
 )

@dae
Copy link
Member

dae commented Apr 15, 2021

I got the error on this Mac - I suspect it would have been reproducible with a clean build, but that would be a pain. Linux's sandboxing is also better than the Mac sandboxing, so the Linux CI can sometimes catch things the others don't.

Those priorities sound fine, if you can get this passing and the colours fixed, I'll try to merge this tomorrow

This was an experiment, to adjust the field border-radius to the
buttons, but I think it looks cleaner if the fields are square
@glutanimate
Copy link
Contributor

glutanimate commented Apr 15, 2021

Can confirm that the scrollbars are gone on Linux. Thanks!

Great work in general, also. Having used the toolbar for a bit now, it feels very polished already.

From a dev standpoint, I especially appreciate all the effort you've put towards fleshing out the add-on API. I still have to give it a spin, but it looks very promising.

A couple of quick observations while using add-ons, perhaps these would be good pickings for an add-on focused follow-up:

  • It would be nice if add-on buttons followed the same styling as the defaults. This is not super important, and I'm not sure if there are any limitations at play here with regards to RawButton and using arbitrary HTML, but it would be nice for consistency:
    Screenshot_20210415_172041 (since it might take a while until add-on authors switch to the new APIs)

  • I noticed that add-on buttons would occasionally not render / remain hidden. From my testing so far I haven't been able to identify any clearly reproducible steps. The CLI output and JS console are quiet as well. You can see what this looks like in practice in this screencast (towards the beginning and end):

Screencast-2021-04-15_17.05.37.mp4

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

@glutanimate
Concerning the button behavior:
I have observed this myself. I think that happens, when you have try to access the button from within the add-on, e.g. document.getElementById("myButton"), which now might fail, because the Svelte might not have rendered the buttons yet, which with the HTML injection was a given.

I'm not sure if there are any limitations at play here [concerning styling]

Svelte puts a lot of focus on local SCSS for maintainability, which means their bound to components. However we could add some global button styling into legacy.scss, which could be used with RawButton

@glutanimate
Copy link
Contributor

Concerning the button behavior:
I have observed this myself. I think that happens, when you have try to access the button from within the add-on, e.g. document.getElementById("myButton"), which now might fail, because the Svelte might not have rendered the buttons yet, which with the HTML injection was a given.

Hmm, interesting. Would definitely make sense as one scenario where this appears.

However, in the case above it's just the plain AnkiWeb version of Image Occlusion, and its only interaction with the button API is via the Python interface, i.e.:

    b = editor.addButton(
        icon,
        _("I/O"),
        lambda o=editor: onImgOccButton(o),
        tip="{} ({})".format(tt, hotkey),
        keys=hotkey,
        disables=False,
    )
    buttons.append(b)

So maybe there's a different race scenario at play also?

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 15, 2021

The new colors are also not a perfect match, but close enough.

The Qt buttons have a box shadow on the bottom edge, which I don't want to recreate, as a box shadow is used to indicate the impressed (active) state.

Screenshot 2021-04-15 at 18 54 26

Screenshot 2021-04-15 at 18 52 50

However I could also move the AddCards dialog into the the toolbar in the next PR, making a custom FullWidthButtonGroup, and call addcards.deck_chooser.choose_deck() or addcards.show_notetype_selector() via bridgeCommands. I've added it to the list above of follow-up PRs.

@dae
Copy link
Member

dae commented Apr 15, 2021

Looks good enough to me!

The missing icon reproduces for me every time after starting Anki + opening the editor. Reopening the editor, the icon is there. this.buttons seems to be undefined on the first call, presumably due to the delay in setupI18n. Maybe we should either retry after a delay, or expose a hook/promise?

    addButtonGroup(newGroup: ButtonGroupProps, group: string | number = -1): void {
        this.buttons?.update((buttonGroups) => {

@hgiesel
Copy link
Contributor Author

hgiesel commented Apr 16, 2021

There was indeed a race condition going on. I think if two asynchronous evals both called this.buttons.update, it also caused a race condition, where the new button would appear for a split second and disappear again. After introducing the promise, I couldn't reproduce this issue anymore, testing with my add-ons, and also IOE, as well as in the browser.

@hgiesel hgiesel marked this pull request as ready for review April 16, 2021 00:09
Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Thanks Henrik! 🚀


righttopbtns_defs = "\n".join(
[
f"{{ component: editorToolbar.RawButton, html: `{button}` }},"
Copy link
Member

Choose a reason for hiding this comment

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

This'll break if there's a backtick in the HTML - safer would be to encode the string with json.dumps()

class:active
class:dropdown-toggle={dropdownToggle}
class:btn-day={!nightMode}
class:btn-night={nightMode}
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, is there any advantage to having a separate day class instead of day being the default, with night mode overriding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, styles defined via classes have higher priority than styles defined via elements. However, with svelte, functionally, it wouldn't actually matter because of the Svelte hashed classes. For debugging purposes it would be nicer to leave both classes:

image


const nightMode = checkNightMode();

const { loading, value: i18n } = useAsync(() => setupI18n());
Copy link
Member

Choose a reason for hiding this comment

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

Is this dead code? If not, setupI18n() call signature needs updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. I abandoned this, once I realized I had to do the i18n setup outside of Svelte.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants