Basic Mechanics #20

vvvyyynet · 2025-02-20T20:18:30Z

For this PR I refactored the single-file test-route from Tuesday aiming to simplify reuse of the floatingApparatus in other projects (open for other names). Below are some comments

Comments and Questions

Where to place clickListeners: Currently I use inconsistent practice: handleNoteClick is attached to every single note whereas handleMarkClick is attached to the mainText-column relying on event-bubbling. What's your opinion on this? It also makes sense.
svelte-ignore a11y: I used svelte-ignore for a11y things a few times to avoid adding aria-types on non-buttons... I think it is ok, since the click-actions are purely visual. But I'd like to have a second opinion on this.
Placing the trigger of Tailwind-Popover: I would like to use a Tailwind Popover for the <MultiMarkPopup>, but I don't understand how to place its trigger...
Getting rid of global styles: When I remove :global() in the css-styles it does not see the DOM-elements inside the {@html}. Another option would be to select for e.g. #container_text, but how to do this using the ids in constants.js? Or would you prefer doing the conditional formatting with .highlighted-classes in pure tailwind?
Loading data on localhost: I tried to externalise the json-fetches (like we did in Parzival with metadata). However, since I use local .json files and no server-API I get the following error: "Cannot use relative URL (/json/aco-text.json) with global fetch — use event.fetch instead: https://svelte.dev/docs/kit/web-standards#fetch-apis"

pdaengeli · 2025-02-20T21:38:17Z

Not a qualified reviewer for this kind of PR, but I assume it is about the comments in a grid column (in ACO displayed on the right of the text), isn't it?

floatingCommentary or floatingAnnotations might be a bit more generic in that case than floatingApparatus (for ACO floatingApparatus is not a bad choice, though).

flicksolutions · 2025-02-21T15:34:22Z

Where to place clickListeners:

*Currently I use inconsistent practice: handleNoteClick is attached to every single note whereas handleMarkClick is attached to the mainText-column relying on event-bubbling. What's your opinion on this? It also makes sense.

From the fact, that svelte started yelling at you, you can gather, that the handleMarkClick-way is the wrong way to go ;-)
Attaching a click handler as narrowly as possible makes more sense, in my opinion. And then attach a "no mark clicked"-Handler to the body itself.

svelte-ignore a11y:

*I used svelte-ignore for a11y things a few times to avoid adding aria-types on non-buttons... I think it is ok, since the click-actions are purely visual. But I'd like to have a second opinion on this.

I have to side with svelte on this: the notes at least are clearly buttons, not just a div! A click on them changes the content of the site. One might even say that the spans are actually buttons, or at least anchors, because stuff happens when you click on them.

Placing the trigger of Tailwind-Popover:

*I would like to use a Tailwind Popover for the <MultiMarkPopup>, but I don't understand how to place its trigger...

Why not use Floating UI svelte? https://next.skeleton.dev/docs/integrations/popover/svelte/

Getting rid of global styles:

*When I remove :global() in the css-styles it does not see the DOM-elements inside the {@html}. Another option would be to select for e.g. #container_text, but how to do this using the ids in constants.js? Or would you prefer doing the conditional formatting with .highlighted-classes in pure tailwind?

I dislike the current use of a mix of CSS and tailwind. You will have to set some styles in the CSS-Section of the svelte files, but this doesn't mean you have to stop using tailwind. Use the @apply-keyword in the CSS section to still apply tailwind.

you can narrow down the global styles a lot if you select an element that is accessible in the code and then apply the :global()-pseudo-selector afterwards. e.g.:
.notebox :global(a) {
instead of
:global(.notebox a) {

and never use purely global styles in the style-section of a svelte file. like :global(p) {. if you have global styles, write them into a external css (postcss) file.

Loading data on localhost:

*I tried to externalise the json-fetches (like we did in Parzival with metadata). However, since I use local .json files and no server-API I get the following error: "Cannot use relative URL (/json/aco-text.json) with global fetch — use event.fetch instead: https://svelte.dev/docs/kit/web-standards#fetch-apis"

This sent me a bit back into my rabbithole of where and when to fetch... But the answer might be simpler in this case: since this is local data, we can just import instead of fetch?! Why are those jsons in the static folder instead of in $lib/data directly? I think this is overcomplicating things... butting the jsons into the data-folder directly and just importing them in the load function might be by far the best solution for this...

flicksolutions

most importantly: your app doesn't work on dark-mode at the moment. you can force the light mode if you only want one mode (https://next.skeleton.dev/docs/guides/mode). but at the moment in dark mode your text basically vanishes.

another very general thing is, that I think you don't use the full potential of svelte when manipulating the DOM:
In most of your functions you are tediously selecting elements from the document. If you had a binding for your element, you could just pass it to the function.
you could build yourself an object with the id's of the notes as keys and the dom-nodes themselfes as values. With a simple bind:this={notes[id]} you would have a lasting reference to the DOM-node.
Of course it would be a little bit more complicated because you created a component for the note, but still very much doable. this would get rid of most of the document.querySelector-calls in your functions.

flicksolutions · 2025-02-21T15:39:35Z

src/lib/functions/floatingApparatus/index.js

can you explain why this file is necessary? I think it's what breaks my intellisense:

The idea of the index is as follows:
(1) It allows me to import the functions as if the floatingApparatus were an external library, i.e. all functions directly from the parent folder, without explicitly naming the individual files.
(2) I like to visually group similar functions together and make some general comments on their usage, independent of the alphabetic order of their names. It's a bit like combining the benefits of single-file functions with grouping similar stuff.

However I understand, if it seems a bit overkill here... would you like to delete the index?

Btw. I found another huge advantage of single-file functions: they have independent file history!

In my VSCodium I don't see the second line handleNoteClick: undefined. No clue where this comes from, especially, since the first box seems to know that the function returns void. So your intellisense does see the function.

flicksolutions · 2025-02-21T15:40:36Z

src/lib/data/content.js

as discussed, this is not necessary. just move the files here and then import them in the load-function

Done. However, I left the imports in content.js which I plan to use as a central file for file imports. I find this easier than to update multiple load-functions if I want to switch APIs.

So, I did move the .json files to the lib/data folder, which indeed is nice for import. However, I wanted to ask you how you differenciate between the static folder and a lib/data folder. I mean, the data is static, too... Do you only use it for media but not for structured data?

flicksolutions · 2025-02-21T15:42:07Z

src/lib/functions/floatingApparatus/getInstantOffsetTop.js

Floating UI Svelte can do this for you can do this for you

Oh, this is nice! I'll implement this in March, when I'll polish the design.

flicksolutions · 2025-02-21T16:09:18Z

src/routes/edition/[vol]/[doc]/[unit]/Note.svelte

+		`notebox word-wrap absolute max-w-[700px] rounded-md border-4 bg-white p-3 transition-transform duration-500`,
+		selectedNote.id === note.id && 'highlighted'
+	]}
+	style={`margin-top:${MARGIN_NOTEBOX}px; margin-bottom:${MARGIN_NOTEBOX}px`}


why do you set this as a style and don't use tailwind?
is for some reason my-2 is not good enough and you need exactly 10 px and you reuse the 10px value often you can set custom styles

tailwind-classnames composed out of fragments won't render due to treeshaking.

[Npx] is just a personal habit, since I always forget which numbers are valid custom styles and which are not. There's no meaning. Note that I'll do styling in March, so here it just has to work.

flicksolutions · 2025-02-21T16:09:41Z

src/routes/edition/[vol]/[doc]/[unit]/Note.svelte

+
+<!-- svelte-ignore a11y_click_events_have_key_events -->
+<!-- svelte-ignore a11y_no_static_element_interactions -->
+<div


svelte is right here. this is a button and not just a div

I also want to get rid of the ignore statements. However, the notes cannot be buttons, since buttons are not permitted to have interactive content. Already now, there are anchor-tags inside the notes for the references. Also think of a nested button that (un)collapses a very long note.

I see two workarounds:

adding aria-roles etc. to the parent div

adding a full-sized invisible button as a sibling of the note-content.

I chose (1) . As I was trying (2) I ran into difficulties related to event-propagation and sizing of the box. It should work in theory, but it isn't optimal either.

src/lib/functions/protoHTMLconversion/generateMainText.js

flicksolutions · 2025-02-21T16:54:09Z

src/lib/functions/protoHTMLconversion/generateLineNumbers.js

using regex to match tags in the content also feels inherently wrong. It might be smarter to create a custom dom and then use the standard way to traverse a DOM-tree. You can use a multitude of ways for that. I also remember there being a npm-package for using it in node-environments. might be a worth a look

(On a sidenote: I actually did use insertAdjacentHTML() in an older version to temporarily insert <span>s that wrap text of selected notes. But then I realised, that I can just add classes to the existing spans.)

Ok, so you suggest to use DOM-manipulation in the conversion form Proto-HTML --> Text/LineNums/PageNums. Now, I see why this is semantically more straight forward. But from a performance point of view, do you think it is wise to create the full DOM tree three times, just to dump half of the nodes right away? My gut feeling says that regexp is less work for the client.

Note, that in the ProtoHTML everything is still contained and then with regexp I'm basically filtering out the parts that are needed for the respective columns, while keeping the <br />-tags in all columns, which will guarantee alignment of the lines.

Of course we could pre-process the protoHTML for all three columns, and everything would be fine :-)

hmm... it would need texting as to what's more performant... both options are quite expensive so it might be worth a test.

src/routes/edition/[vol]/[doc]/[unit]/Text.svelte

src/routes/edition/[vol]/[doc]/[unit]/+page.js

Will be replaced by floatingUI Svelte later

vvvyyynet

Thank you for your detailed review @flicksolutions. I solved the easy tasks and asked some questions, as long as things are still fresh. Some bigger refactoring tasks I leave for March.

1/2 -- Clicklisteners/a11y: I started a discussion further below.
3 -- Popovers: I'll implement Floating UI Svelte in a later PR. Thanks.
4 -- Global CSS-Styles: I dislike the current tailwind/css mix as well and will use as much tailwind as possible inMarch.
5 -- Loading: I'm now using import instead of fetch.

vvvyyynet · 2025-02-21T21:39:21Z

src/routes/edition/[vol]/[doc]/[unit]/Note.svelte

+
+<!-- svelte-ignore a11y_click_events_have_key_events -->
+<!-- svelte-ignore a11y_no_static_element_interactions -->
+<div


I also want to get rid of the ignore statements. However, the notes cannot be buttons, since buttons are not permitted to have interactive content. Already now, there are anchor-tags inside the notes for the references. Also think of a nested button that (un)collapses a very long note.

I see two workarounds:

adding aria-roles etc. to the parent div

adding a full-sized invisible button as a sibling of the note-content.

I chose (1) . As I was trying (2) I ran into difficulties related to event-propagation and sizing of the box. It should work in theory, but it isn't optimal either.

src/routes/edition/[vol]/[doc]/[unit]/+page.svelte

vvvyyynet · 2025-02-23T17:06:09Z

src/lib/data/content.js

Done. However, I left the imports in content.js which I plan to use as a central file for file imports. I find this easier than to update multiple load-functions if I want to switch APIs.

vvvyyynet · 2025-02-23T17:09:49Z

src/lib/data/content.js

So, I did move the .json files to the lib/data folder, which indeed is nice for import. However, I wanted to ask you how you differenciate between the static folder and a lib/data folder. I mean, the data is static, too... Do you only use it for media but not for structured data?

vvvyyynet · 2025-02-23T17:11:46Z

src/lib/functions/floatingApparatus/getInstantOffsetTop.js

Oh, this is nice! I'll implement this in March, when I'll polish the design.

vvvyyynet · 2025-02-23T19:05:52Z

src/lib/functions/protoHTMLconversion/generateMainText.js

+			if (startId) {
+				// Handle <note_start> tag
+				openIds.push(startId);
+				result += `<note_start note_id='${startId}'></note_start>`;


Thanks for being picky on this. I changed it.
Small follow-up: What are your thoughts about <span type="start"> vs. <span class="note-start">?

vvvyyynet · 2025-02-23T19:20:34Z

src/routes/edition/[vol]/[doc]/[unit]/Note.svelte

+		`notebox word-wrap absolute max-w-[700px] rounded-md border-4 bg-white p-3 transition-transform duration-500`,
+		selectedNote.id === note.id && 'highlighted'
+	]}
+	style={`margin-top:${MARGIN_NOTEBOX}px; margin-bottom:${MARGIN_NOTEBOX}px`}


tailwind-classnames composed out of fragments won't render due to treeshaking.

[Npx] is just a personal habit, since I always forget which numbers are valid custom styles and which are not. There's no meaning. Note that I'll do styling in March, so here it just has to work.

src/routes/edition/[vol]/[doc]/[unit]/Text.svelte

vvvyyynet · 2025-02-24T06:08:57Z

src/lib/functions/protoHTMLconversion/generateMainText.js

+				let idsAttribute = JSON.stringify(openIds);
+				if (openIds.length > 0) {
+					let multiIdsClass = openIds.length > 1 ? 'multiple-ids' : '';
+					result += `<span class='mark ${multiIdsClass}' note_ids=${idsAttribute}>${precedingText}</span>`;


@flicksolutions How would you insert an onclick-handler here the svelte way?

I would add an action with use on the div wrapping the maintext in Text.svelte on line 6:

const addSpanHandlers: Action = (node) => { // the node has been mounted in the DOM $effect(() => { // setup goes here node.querySelector(`span`).forEach(n => n.addEventListener("click",eventhandler)) return () => { // teardown goes here node.querySelector(`span`).forEach(n => n.removeEventListener) }; }); };

something along those lines

vvvyyynet · 2025-02-24T06:11:45Z

Oh yes, importantly, in this PR I did not focus much on styling, which is why I renamed it from "Groundwork" to "Basic Mechanics". Styling, dark mode etc. will be a seperate task for March.

vvvyyynet · 2025-02-24T06:12:49Z

Also, I still have to type everything... but since this would require renaming all the files from .js to .ts, I think it's better to do this not during a PR?!

flicksolutions · 2025-02-25T08:28:54Z

Also, I still have to type everything... but since this would require renaming all the files from .js to .ts, I think it's better to do this not during a PR?!

jup, that's fine. Tell me, if you found a simple way to do this. I have a lot of projects I would like to convert to TS but I'm afraid of very tedious work...

vvvyyynet added 6 commits February 16, 2025 17:23

feat: working test-env

0ab1cdf

fix: improve line numbers

6a2b5db

feat: clickable marks

48213f5

fix: various improvements to the test_env

3e390ab

chore: complete rewrite with routing, functions and components

c8fa7f3

fix: move copyWithoutLinebreaks to global state

0a36e1b

vvvyyynet requested a review from flicksolutions February 20, 2025 20:18

flicksolutions requested changes Feb 21, 2025

View reviewed changes

vvvyyynet added 3 commits February 21, 2025 22:43

fix: click on line numbers now inserts URL parameter

1820139

fix: insert aria-roles to notes and remove ignores

20d0d36

fix: import local json-data instead of fetch

8b3bdb1

vvvyyynet changed the title ~~Groundwork~~ Basic Mechanics Feb 23, 2025

vvvyyynet added 5 commits February 23, 2025 18:45

fix: placement of popup

c3df1d3

Will be replaced by floatingUI Svelte later

fix: use tick() instead of timeout for popup

ec79d30

fix: remove invalid note_start/end and use data-* attributes

2113660

fix: reduce globals in style-blocks

360f931

fix: add ARIA-properties instead of svelte-ignore for mainText.

47ae253

vvvyyynet commented Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Mechanics #20

Basic Mechanics #20

vvvyyynet commented Feb 20, 2025 •

edited

Loading

pdaengeli commented Feb 20, 2025

flicksolutions commented Feb 21, 2025 •

edited by vvvyyynet

Loading

flicksolutions left a comment

flicksolutions Feb 21, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

flicksolutions Feb 21, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

flicksolutions Feb 21, 2025

vvvyyynet Feb 23, 2025

flicksolutions Feb 21, 2025

vvvyyynet Feb 23, 2025

flicksolutions Feb 21, 2025

vvvyyynet Feb 21, 2025

flicksolutions Feb 21, 2025

vvvyyynet Feb 23, 2025

flicksolutions Feb 24, 2025

vvvyyynet left a comment

vvvyyynet Feb 21, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 23, 2025

vvvyyynet Feb 24, 2025

flicksolutions Feb 24, 2025

vvvyyynet commented Feb 24, 2025

vvvyyynet commented Feb 24, 2025

flicksolutions commented Feb 25, 2025

Basic Mechanics #20

Are you sure you want to change the base?

Basic Mechanics #20

Conversation

vvvyyynet commented Feb 20, 2025 • edited Loading

Comments and Questions

pdaengeli commented Feb 20, 2025

flicksolutions commented Feb 21, 2025 • edited by vvvyyynet Loading

flicksolutions 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

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

vvvyyynet 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

vvvyyynet commented Feb 24, 2025

vvvyyynet commented Feb 24, 2025

flicksolutions commented Feb 25, 2025

vvvyyynet commented Feb 20, 2025 •

edited

Loading

flicksolutions commented Feb 21, 2025 •

edited by vvvyyynet

Loading