Plugin factory poc #2242

MaPoKen · 2024-02-16T15:17:45Z

Plugin factory: Samle felles plugin logikk og gjør plugins mer debuggable

Kodemessig blir det ikke mindre kode men det skal heller ikke bli så mye mer!

Hovedtanken med factory er å gi oss bedre mulighet til å debugge koden ved normalization/keyevents, men også introdusere en enklere måte å lage felleskode til pluginsene våre.

Største forandringen er hvordan vi normaliserer. Før så ville vi ha en stor metode med ørten if setninger som gjorde readability dårlig, tanken nå er at vi har en array med et normalize objekt. Normalize objektet inneholder en funksjon normalize som er som regel en if funksjon og description som forklarer hva normalize metoden skal gjøre. Håpet er at dette enforcer forklaringer av normaliseringer og gjør normaliseringer litt mer readable.

Ideèr som har blitt innført:

Normaliseringen breaker ikke nå ved første "hit" for en gitt plugin den vil gjennomføre hele normaliseringen for et elementet før den eventuelt returnerer void. NB: dette må nok testes godt men jeg tenker nå at det burde spare oss noen iterasjoner gjennom koden å kunne normalisere alt, worst case er det en enkel fiks

KOM GJERNE MED IDEER 👯

Jonas-C · 2024-02-16T15:23:47Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+        const defaultNormalized = normalizerConfig ? defaultBlockNormalizer(editor, entry, normalizerConfig) : false;
+        const normalized = normalize?.reduceRight((acc, { description, normalize }) => {
+          const isNormalized = normalize(entry as NodeEntry<T>, editor);
+          if (process.env.DEBUG_SLATE && isNormalized) {


Legg heller dette inn i config. Process er egentlig ikke definert på klient. Denne pakken skulle egentlig vært fjernet.

Eller, kanskje vi kan tåle at denne ligger i import.meta.env? Har prøvd å holde meg unna den, men i dette tilfellet er det jo forsåvidt et "build time-flagg"

Jonas-C · 2024-02-16T15:25:45Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+
+type KeyDown = (e: KeyboardEvent, editor: Editor, nextOnKeyDown?: (event: KeyboardEvent) => void) => void;
+
+const createPluginFactory =


Kan heller exporte denne. Ser at du har importert den med stor C

Jonas-C · 2024-02-16T15:26:14Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+
+    editor.normalizeNode = (entry) => {
+      const [node] = entry;
+      if (Element.isElement(node) && node.type === type) {


Mistenker at dette ikke kommer til å holde. Når vi sjekker etter eksterne embeds sjekker vi for eksempel etter både external og iframe

Hmm, det er jo mega kjipt.
Men ser at de er mega minimalistiske, kan vi ikke bare evt. lage plugins for de?

Nei, egentlig ikke. Man vet ikke om noe er en external eller iframe før man prøver å lagre de.

Det kommer ikke til å bli et problem. Vi kan bare lage External plugin med en child plugin som dekker casene for enten iframe eller external. Bing smack.

Jonas-C · 2024-02-16T15:26:49Z

src/components/SlateEditor/plugins/paragraph/index.tsx

+      },
+    },
+    {
+      description: "Unrwap sibling paragraphs if either has serializeAsText",


Jonas-C · 2024-02-16T15:29:21Z

src/components/SlateEditor/plugins/keyFigure/index.tsx

+  normalizerConfig: {
+    previous: {
+      allowed: afterOrBeforeTextBlockElement,
+      defaultType: TYPE_PARAGRAPH,
+    },
+    next: {
+      allowed: afterOrBeforeTextBlockElement,
+      defaultType: TYPE_PARAGRAPH,
+    },
+  },


Forsåvidt litt ryddig å ikke inline all config. Denne kan fint fortsatt ligge utenfor selve funksjonskallet som den gjorde før. Da blir den litt lettere å lese

Jonas-C · 2024-02-16T15:30:17Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  type: Element["type"];
+  normalize?: Normalize<T>[];
+  normalizerConfig?: NormalizerConfig;
+  onKeyDown?: Record<string, KeyDown>;


Finnes det ikke noe keycode-enum i TypeScript?

Nope, de håndterer det som string...

…ugin-factory-poc

gunnarvelle · 2024-02-26T11:05:40Z

Lurt å merge inn master igjen for å være sikker på at vi ikkje gjeninnfører ting vi har fiksa tidligere.

…ugin-factory-poc

Jonas-C · 2024-02-29T09:16:48Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+    editor.onKeyDown = (e) => {
+      const [entry] = Editor.nodes(editor, { match: (node) => Element.isElement(node) && node.type === type });
+      if (entry) {
+        if (onKeyDown?.[e.key]) {


Tror det blir penere å lagre denne som en variabel, kanskje?

Jonas-C · 2024-02-29T09:29:29Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+    };
+
+    editor.onKeyDown = (e) => {
+      const [entry] = Editor.nodes(editor, { match: (node) => Element.isElement(node) && node.type === type });


Dette synes jeg er en degradering fra nåværende løsning. Da henter vi først ut en verdi dersom vi finner en match på onKeyDown. Nå vil vi hente ut en verdi uavhengig av om en onKeyDown er registrert. Vil tro dette kommer til å påvirke ytelse. I og med at vi jobber med en generator her burde det være en måte å skrive dette på som først utfører arbeidet når vi faktisk trenger verdien.

Jonas-C · 2024-02-29T09:46:03Z

src/components/SlateEditor/plugins/details/index.tsx

-
-      if (detailsNode) {
-        const summaryEntry = getCurrentBlock(editor, TYPE_SUMMARY);
+const onBackspace: KeyDown<DetailsElement["type"]> = (e, editor, entry) => {


Kanskje like greit å slenge inn litt kommentarer her når du først er i gang?

Jonas-C · 2024-02-29T09:47:14Z

src/components/SlateEditor/plugins/details/index.tsx

+export const detailsPlugin = createPlugin<DetailsElement["type"]>({
+  type: TYPE_DETAILS,
+  normalizerConfig: detailsNormalizerConfig,
+  renderLeaf: ({ attributes, children, text }, editor) => {


Denne burde egentlig flyttes til render-delen av pluginen

Med på det 👯

Jonas-C · 2024-02-29T09:47:50Z

src/components/SlateEditor/plugins/details/index.tsx

-        }
-      }
-    }
+    return undefined;


Trenger vi å returnere noe her da?

Jonas-C · 2024-02-29T09:48:49Z

src/components/SlateEditor/plugins/details/index.tsx

+        return !!summaryEntry;
+      },
+      onKeyDown: {
+        [KEY_ENTER]: onEnter,


Ikke fan av navngivning på funksjonene. Det burde være åpenbart at de tilhører to forskjellige plugins.

Jonas-C · 2024-02-29T10:32:35Z

src/components/SlateEditor/plugins/paragraph/index.tsx

+    [KEY_ENTER]: onEnter,
+  },
+  normalize: [
+    {


Er det egentlig noe særlig å tjene på at dette ikke bare er en lang funksjon?

…ugin-factory-poc

MaPoKen · 2024-03-27T11:26:54Z

Legger til noen plugins for å sjekke om det gir noen forskjeller på performance

katrinewi

Jeg syntes denne logikken gjør det litt mer oversiktlig og lettere å se hvilke deler som trengs for å lage en ny plugin når man ikke er bestevenn med slate! 😄

Jeg har ikke testet forskjell i hastigheten, så det burde sikkert bli gjort ganske nøye før man lander på å eventuelt ta det videre

Ellers er det en del ubrukte imports rundt om

katrinewi · 2024-05-07T10:06:42Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  normalize?: Normalize<Extract<Element, { type: T }>>[];
+  normalizerConfig?: NormalizerConfig;


Burde forskjellen på normalize og normalizerConfig vært tydeligere i navngivningen? Jeg syntes ikke det er intuitivt at normalize er spesifikk normaliserings-logikk for en plugin mens normalizerConfig er bruk av default block normalizer ..

katrinewi · 2024-05-07T10:11:41Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  [K in ElementType]: Plugin<K>;
+};
+
+interface Plugin<T extends ElementType = ElementType> {


En tanke: jeg tror det hadde blitt enda mer forståelig ved å legge på en kommentar med beskrivelse av noen av feltene under her, feks isVoid, childPlugins, shouldHideBlockPicker, renderLeaf ..

Gjerne med denne kommentar-syntaxen: /** Kommentar */, så får man hvertfall i vs-code opp beskrivelsen ved hover

Jeg prøvde å legge til noen kommentare, kan hende noen er litt vage men kan jo eventuelt legge til link til info i slate doc 👯

Par av kommentarene kan kanskje utbedres:

isVoid: Denne er upresis. Void elements kan ha children nodes, men forskjellen er om elementet renderes av Slate eller av Elements' render kode. Første setning er good, men kanskje 2. setning istedet burde sagt "A void element is not rendered by Slate, but by the Element's render kode..." og ev. med en henvisning til doc

renderLeaf: Burde kommentaren istedet reflektert at den er for å overstyre heller enn å gjøre et valg? Dersom renderLeaf ikke er satt, vil default node brukes? Så noe i retning av Assign to have the leafNode rendered by a specific JSX.element. Otherwise, a default node will be used

katrinewi · 2024-05-07T10:12:18Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  event: NodeEntry<Extract<Element, { type: T }>>,
+) => boolean;
+
+export const createPluginFactory =


Trenger ikke eksporteres!

katrinewi · 2024-05-07T10:26:30Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  };
+
+export const createPlugin = <T extends ElementType>(data: Plugin<T>) => {
+  const plugins: any[] = [data];


Må denne være any? blir ikke Plugin<T>[] mer riktig?

katrinewi · 2024-05-07T10:43:15Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+      if (entry) {
+        if (onKeyDown?.[e.key]) {
+          if (onKeyDown?.[e.key](e, editor, entry as NodeEntry<Extract<Element, { type: T }>>)) {
+            if (config.debugSlate) {


Er det en grunn til at if-sjekken her er nestet 4 nivåer? (linje 97-100)

Klarte å fjerne en men 3 av de trengs :D 👯

…ugin-factory-poc

… Removed unused imports

rauboti · 2024-05-22T07:03:12Z

src/components/SlateEditor/plugins/PluginFactory.tsx

+  /**
+   *  A method to control if the BlockPickerTrigger should be shown or not given the selection is in a node for this plugin.
+   */
+  shouldHideBlockPicker?: (editor: Editor) => boolean;


Er litt usikker på om jeg liker navnet på denne, synes det fort ser mer komplisert og omstendelig ut når attributter, variabler og lignende begynner på ord som should. Personlig ville jeg gått for showBlockPicker med defaultverdi true, altså motsatt tankegang. Men hideBlockPicker vil også fungere (så logikk ellers ikke må snus) og vil være kortere. Og, imo, hakket mer lesbart når det begynner rett på hide eller show.

…ugin-factory-poc

MaPoKen added 3 commits February 16, 2024 12:47

Minimal working plugin factory. Changed KeyFigure to use pluginfactory

e9899f4

Ignore debug logs

5276f87

Wrote pargraph using PluginFactory

5eb5334

Jonas-C reviewed Feb 16, 2024

View reviewed changes

Touch up

7344bab

MaPoKen force-pushed the plugin-factory-poc branch from 5ca260e to 7344bab Compare February 16, 2024 15:53

MaPoKen and others added 3 commits February 19, 2024 11:57

Intial work nested plugins

807e423

Better generics for plugin factory

d8f56ba

Handle nested plugins.

f17d94b

MaPoKen force-pushed the plugin-factory-poc branch from ae17ec9 to f17d94b Compare February 20, 2024 13:38

MaPoKen added 3 commits February 21, 2024 10:24

Nested plugins on table plugin

9e8bea0

Revert tableplugin changes for now

ec3736d

Made details into factory plugin

fae9871

MaPoKen marked this pull request as ready for review February 22, 2024 09:15

MaPoKen requested review from a team February 22, 2024 09:15

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

546336d

…ugin-factory-poc

MaPoKen added 5 commits February 26, 2024 13:50

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

14e415f

…ugin-factory-poc

Spread paragraphplugin in new places with RichTextEditor

8c904ed

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

b6e5f4a

…ugin-factory-poc

Fixed failing test

a28dd7b

onKeyDown changes

365039d

MaPoKen force-pushed the plugin-factory-poc branch from 2d80a4e to 365039d Compare February 28, 2024 10:29

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

234fd2a

…ugin-factory-poc

Added back important if

4137486

MaPoKen requested a review from Jonas-C February 28, 2024 13:16

Jonas-C reviewed Feb 29, 2024

View reviewed changes

MaPoKen added 4 commits March 25, 2024 14:05

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

acf2233

…ugin-factory-poc

Defintionlist to factory plugin

9c5a2dc

Contactblock to factory plugin

41dbe81

link plugin as factory

0038f37

MaPoKen added 2 commits April 2, 2024 12:38

section plugin to factory plugin

096d832

list plugin to factory

55737a4

MaPoKen force-pushed the plugin-factory-poc branch from f244f31 to 55737a4 Compare April 2, 2024 12:18

Missed toolbarutils

8fe7909

katrinewi reviewed May 7, 2024

View reviewed changes

MaPoKen added 2 commits May 13, 2024 14:53

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

23d10f4

…ugin-factory-poc

Added JSDOC to plugin interface. Renamed variables for clarification.…

6ad59df

… Removed unused imports

rauboti reviewed May 22, 2024

View reviewed changes

Merge branch 'master' of github.com:NDLANO/editorial-frontend into pl…

8decf63

…ugin-factory-poc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin factory poc #2242

Plugin factory poc #2242

MaPoKen commented Feb 16, 2024 •

edited

Loading

Jonas-C Feb 16, 2024

Jonas-C Feb 16, 2024

Jonas-C Feb 16, 2024

Jonas-C Feb 16, 2024

MaPoKen Feb 16, 2024

Jonas-C Feb 19, 2024

MaPoKen Feb 22, 2024

Jonas-C Feb 16, 2024

Jonas-C Feb 16, 2024

Jonas-C Feb 16, 2024

MaPoKen Feb 16, 2024

gunnarvelle commented Feb 26, 2024

Jonas-C Feb 29, 2024

Jonas-C Feb 29, 2024

Jonas-C Feb 29, 2024

Jonas-C Feb 29, 2024

MaPoKen Feb 29, 2024

Jonas-C Feb 29, 2024

Jonas-C Feb 29, 2024

Jonas-C Feb 29, 2024

MaPoKen commented Mar 27, 2024

katrinewi left a comment

katrinewi May 7, 2024

katrinewi May 7, 2024

MaPoKen May 14, 2024

rauboti May 22, 2024

katrinewi May 7, 2024

katrinewi May 7, 2024

katrinewi May 7, 2024

MaPoKen May 14, 2024

rauboti May 22, 2024


		type KeyDown = (e: KeyboardEvent, editor: Editor, nextOnKeyDown?: (event: KeyboardEvent) => void) => void;

		const createPluginFactory =

		normalize?: Normalize<Extract<Element, { type: T }>>[];
		normalizerConfig?: NormalizerConfig;

Plugin factory poc #2242

Are you sure you want to change the base?

Plugin factory poc #2242

Conversation

MaPoKen commented Feb 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarvelle commented Feb 26, 2024

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

MaPoKen commented Mar 27, 2024

katrinewi left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaPoKen commented Feb 16, 2024 •

edited

Loading