Added paper details to Paper View #144

Slartibartfass2 · 2024-12-18T07:24:08Z

Closes #41

What I have made

The main change of this PR is that PaperDetailsCard.svelte now displays all the information, planned in the design
- initially only the paper title, authors, year and publisher are shown
- the publication type, publication name and external ID can be shown, when pressing 'show more information'
- by default all inputs are made read-only, but there are two edit mode buttons to toggle that property
For that functionality, I added a ToggleableInput component, which is an auto-sizing textarea
- it's a textarea because it can handle multiple lines
- it's auto-sizing because that's what common textareas can't do (we use an action for that)
I made most paper related components async because the data won't be there instantly
- all of them use skeletons while loading
For a long time, I had the problem that the abstract textarea won't stop expanding when it reaches the end of the card, after trying many solutions with no success, I implemented a semi-hard-coded solution that works
This solution is a svelte action applied to the textarea
- on creation, on update, when input is added to the textarea or when the screen size changes the action is executed
- the action calculates how much space is left between the top of the textarea and the end of the card as it's supposed to be, this distance is added as max-height attribute
- when exceeding this height, a scrollbar is added to the abstract

Checklist

Either tick or cross out the items that do not apply (using ~~example text~~) and give a reason why the item does not apply.

I have updated the documentation accordingly and commented my code
I have added tests that prove my fix is effective or that my feature works
- ~~[ ] unit tests~~ no logic that be tested by a unit test added
- integration tests
- ~~[ ] end-to-end tests~~ not available yet
(for reviewer) I have checked the implementation against the requirements

github-actions · 2024-12-18T07:25:15Z

LCOV of commit `c0b18be` during Code Quality Checks #522

Summary coverage rate:
  lines......: 68.9% (604 of 876 lines)
  functions..: 43.9% (36 of 82 functions)
  branches...: no data found

Files changed coverage rate:
                                                                                                  |Lines       |Functions  |Branches    
  Filename                                                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================================================
  src/lib/components/composites/PaperBookmarkButton.svelte                                        |10.3%     29| 0.0%     1|    -      0
  src/lib/components/composites/input/AbstractToggleableInput.svelte                              |33.3%      3|    -     0|    -      0
  src/lib/components/composites/input/ToggleableInput.svelte                                      |11.1%      9|    -     0|    -      0
  src/lib/components/composites/input/abstract-max-height-action.ts                               |16.7%     30| 0.0%     5|    -      0
  src/lib/components/composites/navigation-bar/PaperNavigationBar.svelte                          | 100%      1|    -     0|    -      0
  src/lib/components/composites/navigation-bar/ProjectNavigationBar.svelte                        | 6.2%     16|    -     0|    -      0
  src/lib/components/composites/navigation-bar/SimpleNavigationBar.svelte                         |16.7%      6|    -     0|    -      0
  src/lib/components/composites/paper-components/PaperInfo.svelte                                 |14.3%      7|    -     0|    -      0
  src/lib/components/composites/paper-components/PaperListEntry.svelte                            | 6.7%     30| 0.0%     1|    -      0
  src/lib/components/composites/paper-components/paper-view/PaperDetail.svelte                    |50.0%      2|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/PaperView.svelte                      |11.1%      9|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/cards/PaperCard.svelte                |20.0%      5|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/cards/PaperCardContent.svelte         | 100%      1|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/cards/PaperDetailsCard.svelte         | 9.8%     51| 0.0%     4|    -      0
  src/lib/components/composites/paper-components/paper-view/decision-buttons/AcceptButton.svelte  |25.0%      4|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/decision-buttons/DecisionButton.svelte|18.2%     11| 0.0%     1|    -      0
  src/lib/components/composites/paper-components/paper-view/decision-buttons/DeclineButton.svelte |25.0%      4|    -     0|    -      0
  src/lib/components/composites/paper-components/paper-view/decision-buttons/MaybeButton.svelte   |25.0%      4|    -     0|    -      0
  src/lib/components/composites/tabs/UnderlineTabsList.svelte                                     |25.0%      4|    -     0|    -      0
  src/lib/controller/paper-controller.ts                                                          |    -      0|    -     0|    -      0
  src/lib/controller/project-controller.ts                                                        |    -      0|    -     0|    -      0
  src/lib/resource.svelte.ts                                                                      | 7.1%     14|    -     0|    -      0

Due to not finding any other solution I decided to write an action that automatically calculates the max height according to the screen height and the position of the text area. This is updated whenever the screen size changes.

luca-schlecker

The page looks great, nice work. 👍

I stumbled upon issues with the integration tests though, I attached some details in the form of a log.
The pipeline doesn't seem to encounter the same problem.

Due to the upcoming gRPC work, I did not review changes regarding controllers or backend APIs.

log.txt

luca-schlecker · 2025-01-21T18:32:40Z

src/lib/components/composites/PaperBookmarkButton.svelte

@@ -7,15 +7,17 @@
    import Tooltip from "./Tooltip.svelte";

    interface Props {
-        paperId: number;
+        loadingPaperId: Promise<number>;


What's your reasoning for pulling that promise inside this component?
Couldn't this component be used as is when wrapped in svelte's {#await ...}?

This question arises multiple times throughout the code, one comment thread is enough though imo.

I used this because the component does not heavily rely on the asynchronous loading, i.e., we can just display the component and as soon as the paper is loaded the functionality works as well.
Therefore, wrapping the component in an await block seemed too much to me.

Where else did you notice this?

luca-schlecker · 2025-01-21T19:01:28Z

src/lib/components/composites/input/ToggleableInput.svelte

+    use:autosize
+    use:inputAction={inputActionProps}


Do you think this workaround is necessary? The abstract's textarea could instead fill the remaining space. It would reduce complexity with the caveat of always filling the remaining space even when provided with a short text (see image below). Visually there wouldn't be too much of a difference with that one exception.

What did you use to produce that?

I tried plenty of approaches and none worked when the text overflows. The action was some sort of last resort.

luca-schlecker · 2025-01-21T19:12:25Z

src/lib/components/composites/paper-components/PaperInfo.svelte

+            {#if "id" in paper}
+                <div class="w-fit text-default-sb-nc text-neutral-500">#{paper.id}</div>
+            {/if}


Is there a reason the id could be missing?

Yes, this is used when the data is only a PaperSpec with no ID. This is the case on the paper create page.

luca-schlecker · 2025-01-21T19:28:37Z

src/lib/components/composites/paper-components/paper-view/PaperDetail.svelte

+
+    type Props = WithElementRef<HTMLAttributes<HTMLDivElement>> & {
+        key: string;
+        value: unknown;


As far as I can tell value is used for class names and content at the same time. This should probably be explained somewhere.

Great catch! I added it to the component documentation.

luca-schlecker · 2025-01-21T19:44:44Z

src/lib/components/composites/paper-components/paper-view/cards/PaperDetailsCard.svelte

+    let basicInfos: BasicInfos = $state({
+        Title: "w-[6rem] sm:w-[7.5rem] md:w-[11rem] lg:w-[19.8rem]",
+        Authors: "w-[4rem] sm:w-[5rem] md:w-[7.3rem] lg:w-[13rem]",
+        Year: "w-[2rem] sm:w-[2.5rem] md:w-[3rem] lg:w-[3.5rem]",
+        Publisher: "w-[5rem] sm:w-[6rem] md:w-[8.6rem] lg:w-[15rem]",
+    });
+    loadingPaper
+        .then((paper) => {
+            basicInfos = {
+                Title: paper.title,
+                Authors: getNames(paper.authors),
+                Year: paper.year?.toString() ?? "N/A",
+                Publisher: "N/A",
+            };
+        })
+        .catch(() => {
+            basicInfos = { Title: "", Authors: "", Year: "", Publisher: "" };
+        });


I recognize a common pattern: Instead of relying on svelte's {#await ...} a state rune is used with a default value. In case you want to avoid {#await ...} (previous comment), I'd recommend simplifying that behavior similar to what is shown here:

// resource.svelte.ts export let resource = <T>( fn: () => Promise<T>, initialValue?: T ) => { const _rune = $state<{ value: T | undefined }>({ value: initialValue }); $effect(() => { fn().then((data) => { _rune.value = data; }); }); return _rune; };

This could also be extended with a default error value. Maybe something like this:

export let resource = <T>( fn: () => Promise<T>, initialValue?: T, onSuccess: (...) => T, onError?: (...) => T ) => { ... }

Nice suggestion, I like it. It's way more clean and descriptive now.

luca-schlecker · 2025-01-21T19:50:08Z

src/lib/components/composites/paper-components/paper-view/cards/PaperDetailsCard.svelte

+                <Skeleton class="flex h-[1.625rem] rounded-full w-[100%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[95%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[70%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[82%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[50%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[75%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[90%]" />


Suggested change

<Skeleton class="flex h-[1.625rem] rounded-full w-[100%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[95%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[70%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[82%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[50%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[75%]" />

<Skeleton class="flex h-[1.625rem] rounded-full w-[90%]" />

{#each [100,95,70,82,50,75,90] as width}

<Skeleton class="flex h-[1.625rem] rounded-full w-[{width}%]" />

{/each}

Good catch. I added it.

luca-schlecker · 2025-01-21T19:54:05Z

...lib/components/composites/paper-components/paper-view/decision-buttons/DecisionButton.svelte

+    let paperId = $state<number | undefined>(undefined);
+    loadingPaperId.then((id) => (paperId = id)).catch(() => (paperId = undefined));


Why not a nullable id?

Please elaborate.

luca-schlecker · 2025-01-21T20:12:55Z

tests/integration/paper-components/paper-view/paper-detail.test.ts

+        expect(screen.queryByTestId("skeleton")).not.toBeNull();
+    });
+
+    test("When paper loading failed without error, then errot text is shown", async () => {


Suggested change

test("When paper loading failed without error, then errot text is shown", async () => {

test("When paper loading failed without error, then error text is shown", async () => {

Good catch. Changed it.

Slartibartfass2

Thank you for the review.

I couldn't reproduce the errors you got in that log. On which commit did you experience that?

Slartibartfass2 · 2025-01-22T10:03:21Z

src/lib/components/composites/PaperBookmarkButton.svelte

@@ -7,15 +7,17 @@
    import Tooltip from "./Tooltip.svelte";

    interface Props {
-        paperId: number;
+        loadingPaperId: Promise<number>;


I used this because the component does not heavily rely on the asynchronous loading, i.e., we can just display the component and as soon as the paper is loaded the functionality works as well.
Therefore, wrapping the component in an await block seemed too much to me.

Where else did you notice this?

Slartibartfass2 · 2025-01-22T10:05:22Z

src/lib/components/composites/input/ToggleableInput.svelte

+    use:autosize
+    use:inputAction={inputActionProps}


What did you use to produce that?

I tried plenty of approaches and none worked when the text overflows. The action was some sort of last resort.

Slartibartfass2 · 2025-01-22T10:06:47Z

src/lib/components/composites/paper-components/PaperInfo.svelte

+            {#if "id" in paper}
+                <div class="w-fit text-default-sb-nc text-neutral-500">#{paper.id}</div>
+            {/if}


Yes, this is used when the data is only a PaperSpec with no ID. This is the case on the paper create page.

Slartibartfass2 · 2025-01-22T10:15:19Z

src/lib/components/composites/paper-components/paper-view/PaperDetail.svelte

+
+    type Props = WithElementRef<HTMLAttributes<HTMLDivElement>> & {
+        key: string;
+        value: unknown;


Great catch! I added it to the component documentation.

Slartibartfass2 · 2025-01-22T10:19:14Z

src/lib/components/composites/paper-components/paper-view/cards/PaperDetailsCard.svelte

+                <Skeleton class="flex h-[1.625rem] rounded-full w-[100%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[95%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[70%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[82%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[50%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[75%]" />
+                <Skeleton class="flex h-[1.625rem] rounded-full w-[90%]" />


Good catch. I added it.

Slartibartfass2 · 2025-01-22T10:24:28Z

...lib/components/composites/paper-components/paper-view/decision-buttons/DecisionButton.svelte

+    let paperId = $state<number | undefined>(undefined);
+    loadingPaperId.then((id) => (paperId = id)).catch(() => (paperId = undefined));


Please elaborate.

Slartibartfass2 · 2025-01-22T10:26:36Z

tests/integration/paper-components/paper-view/paper-detail.test.ts

+        expect(screen.queryByTestId("skeleton")).not.toBeNull();
+    });
+
+    test("When paper loading failed without error, then errot text is shown", async () => {


Good catch. Changed it.

Slartibartfass2 · 2025-01-22T11:27:50Z

src/lib/components/composites/paper-components/paper-view/cards/PaperDetailsCard.svelte

+    let basicInfos: BasicInfos = $state({
+        Title: "w-[6rem] sm:w-[7.5rem] md:w-[11rem] lg:w-[19.8rem]",
+        Authors: "w-[4rem] sm:w-[5rem] md:w-[7.3rem] lg:w-[13rem]",
+        Year: "w-[2rem] sm:w-[2.5rem] md:w-[3rem] lg:w-[3.5rem]",
+        Publisher: "w-[5rem] sm:w-[6rem] md:w-[8.6rem] lg:w-[15rem]",
+    });
+    loadingPaper
+        .then((paper) => {
+            basicInfos = {
+                Title: paper.title,
+                Authors: getNames(paper.authors),
+                Year: paper.year?.toString() ?? "N/A",
+                Publisher: "N/A",
+            };
+        })
+        .catch(() => {
+            basicInfos = { Title: "", Authors: "", Year: "", Publisher: "" };
+        });


Nice suggestion, I like it. It's way more clean and descriptive now.

Those are mainly for the developer and don't provide any advantage for the user

Slartibartfass2 self-assigned this Dec 18, 2024

Slartibartfass2 linked an issue Dec 18, 2024 that may be closed by this pull request

[Use Case]: View Paper Details #41

Open

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch 2 times, most recently from c954292 to 80315d4 Compare December 19, 2024 12:54

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch 2 times, most recently from fbf4abf to bc421b7 Compare December 31, 2024 00:26

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch 5 times, most recently from 006741b to 4f9045f Compare January 9, 2025 18:03

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch 2 times, most recently from ce16465 to c2cd179 Compare January 15, 2025 23:09

Slartibartfass2 requested review from Merseleo and luca-schlecker January 15, 2025 23:38

Slartibartfass2 marked this pull request as ready for review January 15, 2025 23:38

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch 5 times, most recently from dd81310 to 5ffeec4 Compare January 21, 2025 08:57

Slartibartfass2 and others added 5 commits January 21, 2025 09:57

feat: add paper details to paper view

fe91193

feat: make paper view components async

573ea12

feat: add auto max height logic

16c49c2

Due to not finding any other solution I decided to write an action that automatically calculates the max height according to the screen height and the position of the text area. This is updated whenever the screen size changes.

feat: add catch cases to loading paper promises

2488cd8

test: add component tests for paper details card

c7c8eab

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch from 5ffeec4 to c7c8eab Compare January 21, 2025 08:57

luca-schlecker requested changes Jan 21, 2025

View reviewed changes

Merseleo removed their request for review January 21, 2025 21:06

Slartibartfass2 added 2 commits January 22, 2025 12:24

chore: add missing documentation and fix typo

83f7ceb

refactor: use resource wrapper to handle asynchronous resource loading

3ea2a68

Slartibartfass2 commented Jan 22, 2025

View reviewed changes

feat: remove promise rejection errors from being displayed

c0b18be

Those are mainly for the developer and don't provide any advantage for the user

Slartibartfass2 force-pushed the feat/41-use-case-view-paper-details branch from a4ba269 to c0b18be Compare January 22, 2025 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added paper details to Paper View #144

Added paper details to Paper View #144

Slartibartfass2 commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

luca-schlecker left a comment •

edited

Loading

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

luca-schlecker Jan 21, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 left a comment

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

Slartibartfass2 Jan 22, 2025

		let paperId = $state<number \| undefined>(undefined);
		loadingPaperId.then((id) => (paperId = id)).catch(() => (paperId = undefined));

	test("When paper loading failed without error, then errot text is shown", async () => {
	test("When paper loading failed without error, then error text is shown", async () => {

Added paper details to Paper View #144

Are you sure you want to change the base?

Added paper details to Paper View #144

Conversation

Slartibartfass2 commented Dec 18, 2024 • edited Loading

What I have made

Checklist

github-actions bot commented Dec 18, 2024 • edited Loading

LCOV of commit c0b18be during Code Quality Checks #522

luca-schlecker left a comment • 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

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

Slartibartfass2 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

Slartibartfass2 commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

LCOV of commit `c0b18be` during Code Quality Checks #522

luca-schlecker left a comment •

edited

Loading