Preactified popup #1742

StarScape · 2024-04-26T20:15:17Z

This is nowhere near ready, just throwing it up as a draft for visibility, and as a place I can ask questions if I have them :).

Ignore the commit history, I plan to squash this down once it's ready to merge.

birtles

Looking good! I left a few drive-by comments in case they're helpful.

src/content/popup/lang-tag.ts

src/content/popup/KanjiReferencesTable.tsx

StarScape · 2024-04-29T18:52:18Z

Hey @birtles, how do you want to handle theming in the Cosmos fixtures? From a glance, it looks like the fixtures you've previously created for the settings page are affected by the browser theme, but not the 10ten theme, so this is new ground.

I was thinking maybe add a Cosmos decorator that gives us a theme toggle for all fixtures in the content/popup dir, similar to the locale toggle. How does that sound?

EDIT: I went ahead and did this, but of course LMK if you'd prefer a different approach.

birtles · 2024-04-30T02:00:37Z

Hey @birtles, how do you want to handle theming in the Cosmos fixtures? From a glance, it looks like the fixtures you've previously created for the settings page are affected by the browser theme, but not the 10ten theme, so this is new ground.

I was thinking maybe add a Cosmos decorator that gives us a theme toggle for all fixtures in the content/popup dir, similar to the locale toggle. How does that sound?

EDIT: I went ahead and did this, but of course LMK if you'd prefer a different approach.

That's exactly what I had in mind. Looks great!

birtles

Looking great! Thank you!

birtles · 2024-04-30T02:02:35Z

src/common/i18n.tsx

-    () => ({ t, setLocale, locale: props.locale }),
-    [t, setLocale, props.locale]
-  );
+  const langTag = useMemo(() => t('lang_tag'), [t, props.locale])


Nit: GitHub is showing a bunch of prettier errors (missing semicolon in this case). I wonder if you can run prettier on your branch or set up your editor to run it automatically on save?

Yeah, sorry about all the CI failure emails. What's weird is I thought I saw it running prettier as a pre-commit hook—but I guess that was just ESLint? Either way, yeah I can run it locally to avoid the GH errors.

Looks like I only configured it to run on .ts files (not .tsx or .json files etc.)

10ten-ja-reader/package.json

Lines 55 to 58 in 9a0ce77

"*.ts": [

"prettier --write",

"eslint --fix"

],

Ah, that makes sense. I can change it as part of this PR.

Okay, well I did run the lint_staged task before pushing up my latest commit (also updated it to include .tsx files), but I didn't think about the couple of style errors from previous commits that weren't in the staged files, so the pipeline still sent a failure message for that push...but hopefully the last one 😄.

birtles · 2024-04-30T02:12:02Z

src/content/popup/KanjiReferencesTable.fixture.tsx

@@ -0,0 +1,176 @@
+import { KanjiReferencesTable } from './KanjiReferencesTable';
+
+// import '../../../css/popup.css';


So this is super tricky, but it looks like we're using options.html as the template file due to this line here:

10ten-ja-reader/webpack.config.cosmos.js

Line 51 in 9a0ce77

template: './src/options/options.html',

That's why we don't need to import popup.css ourselves.

Ideally we'd use a separate template for popup components vs options components but I don't know if there's a way to get Cosmos to use separate template files based on the fixture. It might be worth having a quick dig (or asking on their discord) but otherwise I guess this is fine as-is.

I can definitely see if I can set up a separate template for options and popup fixtures.

FYI, the commented out line is there because the coloration on the table rows wasn't working, and I thought it was because the popup.css styles weren't getting imported properly. Turns out that it was actually because theme.css hadn't been imported, since that's where the CSS var for --cell-highlight-bg gets defined. Just hadn't gotten to removing it yet :).

FYI, looks like it's not possible to have more than one HTML template. The main React Cosmos maintainer Ovidiu recommended we import the CSS via JS, and use decorators if we need to vary it based on folder.

Thanks for chasing up with Ovidiu about that! Ok, that seems fine. It's not strictly the CSS files that I'm interested in but rather the SVG assets in options.html (which should be available to options components but not to popup componets). Anyway, using options.html as-is seems fine for now. If it becomes a problem we can address it then.

src/content/popup/KanjiReferencesTable.fixture.tsx

src/content/popup/KanjiReferencesTable.tsx

src/content/popup/cosmos.decorator.tsx

src/content/popup/kanji.ts

StarScape · 2024-04-30T18:40:14Z

src/content/popup/KanjiEntry.tsx

+      {options.kanjiReferences && options.kanjiReferences.length && (
+        <KanjiReferencesTable
+          entry={entry}
+          kanjiReferences={options.kanjiReferences}
+        />
+      )}


FWIW, I have very mixed feelings about this style as a way of doing conditional rendering, but I've acquiesced to it over time because it seems to be the accepted practice in the React world. Still, happy to change it depending on your feelings.

Yeah, in general I think it's fine (as long as you ensure the value on the LHS is a bool, otherwise you sometimes get undefined and things showing up in the output). In this case I think !!options.kanjiReferences?.length && ... would do the trick.

StarScape · 2024-04-30T18:43:44Z

src/content/popup/KanjiEntryTopPart.tsx

+    </div>
+  );
+}
+


Not sure what your preference is on helper components being declared above/below the main component. It doesn't particularly matter to me either way, and actually I tend to declare them above, and put the main component at the bottom, but I did the opposite here since that more closely mirrored what you had in the old manual rendering code in kanji.ts.

Thanks. Yeah, these days I lean towards doing the main thing at the top and adding the helpers below (i.e. leaning on JS's function hoisting).

I think of this as being "newspaper style" where you describe the main thing first then add details below. I find this is easier to read since you start reading at the top, get an overview, and then can continue reading for as long as you're interested.

StarScape · 2024-04-30T18:46:50Z

src/content/popup/KanjiEntryTopPart.tsx

+function LegacyIcon({
+  iconSvg,
+  opacity,
+}: {
+  iconSvg: SVGElement;
+  opacity: number;
+}) {
+  const ref = useRef<HTMLDivElement>(null);
+  useLayoutEffect(() => {
+    iconSvg.classList.add('svgicon');
+    iconSvg.style.opacity = `${opacity}`;
+    ref.current?.replaceWith(iconSvg);
+  });
+
+  return <div ref={ref}></div>;
+}


This is a temporary hack I have no intention of actually keeping. I just did it to get the icons rendering onscreen again with the new React setup, without having to change too much code at once.

However, it does raise the question of how you'd like to handle the icons in icons.ts. Should I just convert each of those functions to a React component that returns an <svg>?

However, it does raise the question of how you'd like to handle the icons in icons.ts. Should I just convert each of those functions to a React component that returns an <svg>?

Yes, I think converting those to individual React components makes the most sense.

StarScape · 2024-04-30T18:54:14Z

src/content/popup/expandable.ts

@@ -64,6 +64,9 @@ export function updateExpandable(
      `calc(${round(collapsedHeight, 2)}px + var(--expand-button-allowance))`
    : `${expandedHeight}px`;

+  // TODO: temporary solution in development bc this isn't playing well with React
+  expandable.style.height = '600px';


For some reason, the .expandable element was collapsing to height: 0px whenever I switched the kanji entry to React. I'm not totally sure why, but I'm guessing something to do with the React element not yet being rendered when the expandable does its height measurement. I'll dig into it more tomorrow so I can get rid of this.

Yes, this part is tricky (it took quite a bit of work to get the expandable behavior to behave correctly in all the different configurations) so I'd definitely prefer to deal with this in a separate PR where we can take the time to get it right.

StarScape · 2024-04-30T19:07:07Z

@birtles I remember you had said something about converting just the kanji reference table to React and shipping that first, and we can still do that if you'd like, I'll just have to roll back some commits on this branch.

However, I've went ahead and started on converting the Kanji tab, and I think I might as well get the whole tab Reactified and stable as part of this PR. Actually, unless it turns out to be too much work to bite off at once, I'd like to go ahead and convert the Words and Names tabs too. After that I'll open up a separate draft for migrating to Tailwind.

src/content/popup/KanjiEntryTopPart.tsx

birtles · 2024-05-02T02:22:52Z

@birtles I remember you had said something about converting just the kanji reference table to React and shipping that first, and we can still do that if you'd like, I'll just have to roll back some commits on this branch.

However, I've went ahead and started on converting the Kanji tab, and I think I might as well get the whole tab Reactified and stable as part of this PR. Actually, unless it turns out to be too much work to bite off at once, I'd like to go ahead and convert the Words and Names tabs too. After that I'll open up a separate draft for migrating to Tailwind.

Sorry for the delay, yes, I'd like to just ship the converted kanji table first. There are two risks I'm concerned about:

Preact includes in its code a reference to innerHTML (for implementing dangerouslySetInnerHTML) which some extension store linters may flag as unsafe. I'd like to ship this small change quickly to check we don't get tripped up on any store reviews before going any further. We're already shipping Preact as part of the options page, but the policy applied to content scripts might be more strict.
By bundling Preact into the content script we might adversely affect page load times. It would be good to work this out too before going any further. At least for this risk, however, there are various workarounds we can try like lazily-loading the popup code.

I'd also prefer to do the Preact and Tailwind conversion hand-in-hand (as separate patches/commits but at around the same time) just because it's easier to convert the styles for a component when you have its structure in your head. Which is to say, I don't want to go too far with the other Preact conversion in this PR.

Would you mind narrowing this PR down to just the kanji references table in this PR?

birtles · 2024-05-02T02:25:10Z

package.json

@@ -56,6 +56,10 @@
      "prettier --write",
      "eslint --fix"
    ],
+    "*.tsx": [


I think we can just combine this with the above *.ts pattern to make it *.{ts,tsx}. Maybe even *.tsx? works?

I tried *.tsx? previously and it wouldn't work, but *.{ts,tsx} might do the trick.

Yeah, I noticed the docs have *.{ts,tsx} (or actually I think it was *.{js,jsx}) so at least that much should work. Not sure why *.tsx? doesn't work but I suppose it's due to the glob library they're using.

StarScape · 2024-05-02T03:11:23Z

Sorry for the delay, yes, I'd like to just ship the converted kanji table first. There are two risks I'm concerned about: [...]

Ah okay, I was thinking maybe you were planning to ship just the kanji table first purely because it was an easy first step, didn't realize these other reasons.

Would you mind narrowing this PR down to just the kanji references table in this PR?

Yeah, no problem. I'll do that and move the other work I've done so far to another branch.

StarScape · 2024-05-02T14:12:05Z

Preact includes in its code a reference to innerHTML (for implementing dangerouslySetInnerHTML) which some extension store linters may flag as unsafe.

As a side note, I wonder if webpack will tree-shake this away for us, so long as we don't use dangerouslySetInnerHTML? I have no idea though.

StarScape · 2024-05-02T16:39:07Z

Alright, lets try this on for size now. I've cleaned the commit history to include just the table. I'll tack on the Tailwind conversion of the table styles as a separate commit when I get the chance.

FYI, there is a warning about bundle size for the content script that I don't think was there before:

  WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
  This can impact web performance.
  Assets:
    10ten-ja-content.js (683 KiB)
    10ten-ja-background.js (515 KiB)
    10ten-ja-options.js (354 KiB)
    [...]

I'm guessing that's probably due to Preact being included. Is figuring out whether page load times are affected and if we need to lazy-load the content script something you wanted to do as part of this PR? If so, I'm happy to help, but might need a little guidance. The nuances of web extension development are new to me 🙂.

Also, the knip.js run in failing on CI because KanjiReferencesTable is exported but not imported anywhere in the source (WrappedKanjiReferenceTable is imported instead, to give it access to the i18n context). However, it is imported in the fixture file, which is ignored by knip, so removing the export causes an error there. Kind of a catch-22, not sure how to fix that.

birtles · 2024-05-03T01:59:14Z

Preact includes in its code a reference to innerHTML (for implementing dangerouslySetInnerHTML) which some extension store linters may flag as unsafe.

As a side note, I wonder if webpack will tree-shake this away for us, so long as we don't use dangerouslySetInnerHTML? I have no idea though.

From memory it doesn't get tree-shaken unfortunately (which makes sense since I think tree-shaking is generally pretty limited to things like independently exported functions/variables as opposed to class/component methods).

birtles · 2024-05-03T02:16:57Z

Alright, lets try this on for size now. I've cleaned the commit history to include just the table. I'll tack on the Tailwind conversion of the table styles as a separate commit when I get the chance.

Great, thank you! I think we can do the Tailwind conversion as a separate PR even (see comment below). So once you're happy with the Preact part, feel free to mark this PR as ready for review.

FYI, there is a warning about bundle size for the content script that I don't think was there before:
  WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
  This can impact web performance.
  Assets:
    10ten-ja-content.js (683 KiB)
    10ten-ja-background.js (515 KiB)
    10ten-ja-options.js (354 KiB)
    [...]
I'm guessing that's probably due to Preact being included. Is figuring out whether page load times are affected and if we need to lazy-load the content script something you wanted to do as part of this PR? If so, I'm happy to help, but might need a little guidance. The nuances of web extension development are new to me 🙂.

Normally I try to keep the main branch such that it's always shippable but I think it's fine in this case to merge this and then address the perf issue in a separate PR.

I think what might make sense is to create a async import (annotated with /* webpackChunkName: 10ten-ja-popup */) for the popup code. We might end up creating some sort of popup proxy that takes care of loading the popup code and then calling it. That would allow us to initiate the load from tryToUpdatePopup or somewhere to minimize any delay when we first load it (by overlapping the load with our dictionary lookup).

We can work out the details in a separate PR but I think that would ultimately mean that for users who have the extension permanently loaded (and activated using Ctrl or somesuch) they only pay the cost of loading a more minimal content script on pages where they don't actually look anything up.

So I think what I would want to do is:

Merge this PR
Make a new PR for the lazy loading of the popup code
Make another PR for tailwind conversion of the kanji references table (getting the stylesheet to be correctly generated in all the places we load it could be tricky)
Ship the next version

Also, the knip.js run in failing on CI because KanjiReferencesTable is exported but not imported anywhere in the source (WrappedKanjiReferenceTable is imported instead, to give it access to the i18n context). However, it is imported in the fixture file, which is ignored by knip, so removing the export causes an error there. Kind of a catch-22, not sure how to fix that.

I think we can just mark the export as @public for now: https://knip.dev/guides/handling-issues#unused-exports

StarScape · 2024-05-03T19:37:58Z

Okay, sounds good! I'm happy with this PR whenever you are.

If you have any remaining nits, feel free to commit them yourself—or leave them to me, whatever you prefer. Just putting it out there if you don't want to wait the 12hr time difference for me to wake up and get to my computer again 🙂.

birtles · 2024-05-04T02:30:24Z

Okay, sounds good! I'm happy with this PR whenever you are.

If you have any remaining nits, feel free to commit them yourself—or leave them to me, whatever you prefer. Just putting it out there if you don't want to wait the 12hr time difference for me to wake up and get to my computer again 🙂.

Great, thank you!

I pushed some nits now but unfortunately I found a nasty problem. At the point where we go to calculate the height of the kanji entry (in updateExpandable), the references table has not been rendered. As a result, the height of the popup is too short.

I think we need to force the render to by synchronous. Preact doesn't properly implement flushSync (see preactjs/preact#3929) but we might be able to work around this by overriding debounceRendering as described here: preactjs/preact#4044 (comment)

StarScape · 2024-05-04T13:56:39Z

As a result, the height of the popup is too short.

I hadn't noticed the height discrepancy because I keep my main 10ten installation with the 'always expand window' option checked, but I see what you're talking about now. My bad for not catching that. I'll see if I can get a synchronous render working somehow.

birtles · 2024-05-05T01:29:10Z

As a result, the height of the popup is too short.

I hadn't noticed the height discrepancy because I keep my main 10ten installation with the 'always expand window' option checked, but I see what you're talking about now. My bad for not catching that. I'll see if I can get a synchronous render working somehow.

No problem! For what it's worth you can use yarn start:firefox or yarn start:chrome to run the extension with a fresh profile for testing the default prefs.

I'll have a look at the sync render tomorrow if you don't get to it before then.

Once we've converted the whole thing to Preact we should be able to replace the sync render with a useLayoutEffect or possibly even ResizeObserver so the sync render is just an interim thing.

StarScape · 2024-05-05T03:58:44Z

For what it's worth you can use yarn start:firefox or yarn start:chrome to run the extension with a fresh profile for testing the default prefs.

Oh, I do do this, I just meant that I was using the stable, released version to compare against the dev version (the one running with yarn start:firefox) to check that everything was lining up 1:1, and didn’t realize the behavior of the expandable was off, since I just have the window fully expand on my main installation.

I'll have a look at the sync render tomorrow if you don't get to it before then.

So I did try the trick you linked to above with setting debounceRendering, but it didn’t fix the expandable bug, unfortunately. I dug around for any other ways to force Preact to do a synchronous render but couldn’t find anything at a glance. Digging any further was going to require me to jump into the Preact source to see what I was missing, which I haven’t had time to get into yet.

If you beat me to the punch on figuring it out I won’t complain 😅.

birtles · 2024-05-06T05:19:32Z

So I did try the trick you linked to above with setting debounceRendering, but it didn’t fix the expandable bug, unfortunately.

I had a look at this and found the same thing. I think it must be possible to force Preact to render synchronously somehow because the Preact render tests do it:

https://github.com/preactjs/preact/blob/d18ac9f697e3604495d6496e4973c0af555ae7ba/test/browser/render.test.js#L1171

It looks like their setup is more complex where they override debounceRendering to build up a queue of callbacks and then drain them:

https://github.com/preactjs/preact/blob/d18ac9f697e3604495d6496e4973c0af555ae7ba/test-utils/src/index.js#L7-L11

That said, some tests don't appear to be using the act() or rerender() function and still (presumably) pass so perhaps there's something else going on there.

StarScape · 2024-05-07T01:07:25Z

I tried using their setupRerender() function (and calling the function it returns), but even more confusingly, that made the table fail to render at all.

I also tried using Component.forceUpdate(), but while that did render the table, the bug with expandable was still present—either because the update is still happening asynchronously, ~~or because something else is the issue~~*. I've been a but I may have time to look more into this tomorrow.

* Update: I did some testing just now, and the synchrony (or lack thereof) is definitely the issue.

If you add a setTimeout inin popup.ts where the expandables are updated, like so:

// Collapse expandable containers
for (const expandable of contentContainer.querySelectorAll<HTMLElement>(
  '.expandable'
)) {
  if (expandable.classList.contains('foobar')) {
    setTimeout(() => {
      updateExpandable(expandable, {
        ...options,
        showKeyboardShortcut: options.displayMode === 'static',
      });
    }, 0);
  } else {
    updateExpandable(expandable, {
      ...options,
      showKeyboardShortcut: options.displayMode === 'static',
    });
  }
}

then the issue disappears.

birtles · 2024-05-07T07:30:31Z

Thanks for trying those ideas! I've been away from the office this past week due to family issues but hopefully I'll be back in a couple of days and can look into it more thoroughly then.

Yes, you can confirm the issue in a few ways like printing out the outerHTML of the container after calling render and debugging the values updateExpandables gets.

One alternative might be to pass in a callback and then call it from a useLayoutEffect hook and use that to trigger updateExpandables but I think the popup positioning code might also want the final layout size so if we can keep the render synchronous that would be ideal.

StarScape · 2024-05-07T15:03:10Z

I've been away from the office this past week due to family issues but hopefully I'll be back in a couple of days and can look into it more thoroughly then.

No worries! I've been busy the last few days too like I said, but hopefully I'll have time to jump back into this tomorrow or Wednesday, if you haven't beaten me to it.

One alternative might be to pass in a callback and then call it from a useLayoutEffect hook and use that to trigger updateExpandables but I think the popup positioning code might also want the final layout size so if we can keep the render synchronous that would be ideal.

Yeah, I had considered the useLayoutEffect route. That's a good backup plan if we can't find a way to make a synchronous render work.

This is a wild thought, but we could maybe use Preact's SSR to get a synchronous render. That involves rendering to a string though, so we'd then have to use either innerHTML or a DOMParser, which would probably have security concerns. Not the most elegant solution, either.

StarScape · 2024-05-08T17:50:28Z

Okay, I've finally fixed this!

I still could not get it to render synchronously. However, passing down a callback that updates the .expandable and then calling that inside a useLayoutEffect hook works, and there's no 1-frame flash of incorrectly-sized UI like you get if you do the updateExpandable() call inside of a setTimeout.

FWIW, I think the reason the sync render isn't working as expected might be something to do with the WrappedKanjiReferencesTable and the Provider HOC inside. During my testing, I noticed that a useLayoutEffect put inWrappedKanjiReferencesTable actually fired before one put in KanjiReferenceTable itself. I would think it would be the opposite, since useLayoutEffect is meant for things like measuring rendered sizes of elements, and it’s awfully hard to do that when a component's children aren't rendered yet. But admittedly, I have always been a little fuzzy on when exactly (P)React updates what component, and am only feeling more so after this.

Anyway, it works now. I'm happy with it if you're happy with it.

birtles · 2024-05-09T02:57:11Z

Thanks for working on this!

Unfortunately updateExpandable is not the only code that expects us to have a final layout. getPopupDimensions()—which we use when positioning the popup—also does.

As a result, with the current PR we'll end up positioning the popup in the wrong place in some cases since we won't take into account the size of the references table:

I'm sure we can work around that too but if at all possible I'd like to do a sync render simply because it's less risky. If we end up adding workarounds we might miss something else or cause new issues due to the different timing of the calls (which will be different to what we finally ship anyway).

FWIW, I think the reason the sync render isn't working as expected might be something to do with the WrappedKanjiReferencesTable and the Provider HOC inside.

Oh that's really interesting. I hadn't thought of that!

birtles · 2024-05-09T03:16:50Z

Oh yeah, it's very likely the I18nProvider to blame:

  return (
    <i18nContext.Provider value={value}>
      {!isLoading && props.children}
    </i18nContext.Provider>
  );

Until the layout effect in the provider runs and resolves its async import, isLoading will be true and hence the children won't be rendered.

The next step might be working out how to do a compile-time conditional import of 'webextension-polyfill' so we can avoid the async-ness of I18nProvider when __EXTENSION_CONTEXT__ is true—at least until we have converted everything to Preact (at which point we'll probably switch back to being async so we can use our own locale handling).

birtles · 2024-05-09T04:36:44Z

I went ahead and split up the i18n provider so that it renders synchronously in extension contexts and it seems to work for me over in the sync-i18n branch.

I'm going to cherry pick the i18n refactoring patch and try to land that as a separate PR. If that lands hopefully you should be able to rebase this PR on top of it and drop the bugfix: update expandable after Preact render commit.

birtles · 2024-05-09T04:49:03Z

Ok, I've made the i18n provider sync over in #1759 so you should be able to rebase on top of that and drop the changes to explicitly call updateExpandable.

I added the langTag part to i18n in the process so unfortunately that will cause merge conflicts but hopefully they're not too difficult to resolve 😬

StarScape · 2024-05-09T17:21:43Z

Oh yeah, it's very likely the I18nProvider to blame:

return (
<i18nContext.Provider value={value}>
{!isLoading && props.children}
</i18nContext.Provider>
);

Until the layout effect in the provider runs and resolves its async import, isLoading will be true and hence the children won't be rendered.

Oh, nice catch. Seems kind of obvious now 😅.

Ok, I've made the i18n provider sync over in #1759 so you should be able to rebase on top of that and drop the changes to explicitly call updateExpandable.

Thanks! I've done the rebase.

birtles · 2024-05-10T02:05:10Z

Thanks! I've done the rebase.

Great! Thank you!

StarScape · 2024-05-10T02:53:55Z

Well, with all the back and forth this one took I might not have saved you much time, but I’m glad this finally got merged. Thanks for the patience.

I’m hoping to work on the lazy-loading of the popup code in a few day’s time—hopefully we can get that one sorted out with less than 70 comments on the PR :).

birtles · 2024-05-10T03:25:02Z

Well, with all the back and forth this one took I might not have saved you much time, but I’m glad this finally got merged. Thanks for the patience.

Not at all! Thanks for your patience!

Thanks to you noticing the provider was the problem I think we came up with something that should be pretty low risk to ship.

I’m hoping to work on the lazy-loading of the popup code in a few day’s time—hopefully we can get that one sorted out with less than 70 comments on the PR :).

Yeah, I've been thinking about that. I'm not sure yet how it should work. I was actually going to have a fiddle with it this afternoon if you don't mind.

StarScape · 2024-05-10T03:28:33Z

Yeah, I've been thinking about that. I'm not sure yet how it should work. I was actually going to have a fiddle with it this afternoon if you don't mind.

Oh yeah, by all means. It'll be a lot easier for you to weigh the best options there anyway since this is your codebase.

birtles · 2024-05-15T07:02:41Z

Just to quickly follow up here—the last few days have been very busy so I've only had an hour or so to work on this. I started refactoring the popup positioning code over in the lazy-load-popup branch and hopefully I'll get some dedicated time to work on that tomorrow.

StarScape · 2024-05-15T11:59:54Z

Nice, looking forward to seeing what you come up with.

birtles · 2024-05-16T05:58:35Z

Sorry, looks like it will be Saturday before I can work on this properly.

birtles reviewed Apr 27, 2024

View reviewed changes

src/content/popup/lang-tag.ts Outdated Show resolved Hide resolved

src/content/popup/KanjiReferencesTable.tsx Outdated Show resolved Hide resolved

src/content/popup/KanjiReferencesTable.tsx Outdated Show resolved Hide resolved

birtles reviewed Apr 30, 2024

View reviewed changes

StarScape commented Apr 30, 2024

View reviewed changes

src/content/popup/KanjiEntryTopPart.tsx Outdated Show resolved Hide resolved

birtles reviewed May 2, 2024

View reviewed changes

StarScape force-pushed the chore/convert-popup-to-react branch from 7ce4245 to 3d8069a Compare May 2, 2024 16:23

StarScape force-pushed the chore/convert-popup-to-react branch from 3d8069a to c5a4d4c Compare May 3, 2024 19:26

StarScape marked this pull request as ready for review May 3, 2024 20:01

StarScape and others added 2 commits May 9, 2024 12:05

chore: convert kanji reference table to React component

6cbc847

chore: various review tweaks

b6ff138

StarScape force-pushed the chore/convert-popup-to-react branch from e4873bc to b6ff138 Compare May 9, 2024 17:11

birtles merged commit f0655d6 into birchill:main May 10, 2024
1 check passed

		@@ -0,0 +1,176 @@
		import { KanjiReferencesTable } from './KanjiReferencesTable';

		// import '../../../css/popup.css';

Preactified popup #1742

Preactified popup #1742

Conversation

StarScape commented Apr 26, 2024

birtles left a comment

Choose a reason for hiding this comment

StarScape commented Apr 29, 2024 • edited

birtles commented Apr 30, 2024

birtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarScape Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birtles May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarScape Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarScape Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarScape commented Apr 30, 2024

birtles commented May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarScape commented May 2, 2024

StarScape commented May 2, 2024

StarScape commented May 2, 2024 • edited

birtles commented May 3, 2024

birtles commented May 3, 2024

StarScape commented May 3, 2024

birtles commented May 4, 2024

StarScape commented May 4, 2024 • edited

birtles commented May 5, 2024

StarScape commented May 5, 2024

birtles commented May 6, 2024

StarScape commented May 7, 2024 • edited

birtles commented May 7, 2024

StarScape commented May 7, 2024

StarScape commented May 8, 2024 • edited

birtles commented May 9, 2024

birtles commented May 9, 2024

birtles commented May 9, 2024

birtles commented May 9, 2024

StarScape commented May 9, 2024

birtles commented May 10, 2024

StarScape commented May 10, 2024 • edited

birtles commented May 10, 2024

StarScape commented May 10, 2024

birtles commented May 15, 2024

StarScape commented May 15, 2024

birtles commented May 16, 2024

StarScape commented Apr 29, 2024 •

edited

StarScape Apr 30, 2024 •

edited

birtles May 2, 2024 •

edited

StarScape Apr 30, 2024 •

edited

StarScape Apr 30, 2024 •

edited

birtles commented May 2, 2024 •

edited

StarScape commented May 2, 2024 •

edited

StarScape commented May 4, 2024 •

edited

StarScape commented May 7, 2024 •

edited

StarScape commented May 8, 2024 •

edited

StarScape commented May 10, 2024 •

edited