Added editMessage function by JaBrik228 · Pull Request #9 · SDesya74/notes

JaBrik228 · 2024-07-30T18:33:52Z

No description provided.

SDesya74 · 2024-07-30T18:45:37Z

src/lib/assets/controlIcons/editIcon.svg

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="iso-8859-1"?>


Прогони через svg optimizer чтобы убрал комменты и лишнее

SDesya74 · 2024-07-30T18:57:47Z

src/lib/index.svelte.ts

 }

-export type Message = {
+export interface IMessage {


Не уверен что следует слой логики переименовывать ради фронтенда

SDesya74 · 2024-07-30T18:58:59Z

src/lib/index.svelte.ts

        delete this.inner[id]
    }

+    edit(id: Id, newContent: string) {


А почему бы просто сообщению контент не установить, раз оно в onedit доступно? Реактивное же

SDesya74 · 2024-07-30T19:00:29Z

src/routes/+page.svelte

    }
+
+    function onEditMessage() {
+        if (currEditableMessageId) {


Условие можно инвертировать, тогда вложенность будет меньше

if (!currEditableMessageId) { return }

И имя currEditableMessageId наверное лучше заменить на currentEditingMessage, возможно даже лучше сделать не Id, потому что message - сигнал и по Id его можно не таскать

SDesya74 · 2024-07-30T19:03:29Z

src/routes/+page.svelte

+        }
+    }
+
+    function focusOnMessage(value: IMessage) {


function startEditing мб? Более декларативно звучит и объясняет, зачем нужна функция

SDesya74 · 2024-07-30T19:04:49Z

src/routes/+page.svelte

+        textInputElement?.focus()
+    }
+
+    function onEditMessageByKeyDown(e: KeyboardEvent) {


Та же история, startEditByShortcut или типа того, лучше именовать бизнес функции по смыслу, а не по действию

SDesya74 · 2024-07-30T19:07:48Z

src/routes/+page.svelte


    <footer
-        class="dark:text-white bg-slate-100 dark:bg-slate-700 border-t border-gray-300 dark:border-slate-600 p-4 absolute left-0 bottom-0 w-full"
+        class={`dark:text-white ${currEditableMessageId ? 'bg-orange-100' : 'bg-slate-100'} dark:bg-slate-700 border-t border-gray-300 dark:border-slate-600 p-4 absolute left-0 bottom-0 w-full`}


Тут можно воспользоваться свелтовским синтаксисом

class:bg-orange-100={currEditableMessageId}

А, ну и bg-orange возможно стоит поменять на bg-accent с прозрачностью через bg-opacity. Хотя текущая механика установки цветов прозрачность не поддерживает... В целом можно и bg-orange оставить, но если есть желание прямо в этом ПР починить и прозрачность цветов, можешь это сделать. В файле со стилями темы вроде даже написано как

JaBrik228 · 2024-07-30T22:30:42Z

Нашёл баг: При изменении сообщения, оно не обновлялось в localStorage.
Добавил функцию, в которой происходит запись сообщений в localStorage, чтобы не дублировать код в двух местах, теперь если логика сохранения сообщений измениться, можно будет изменить её в одной функции.

Так же внёс все правки, по твоим замечаниям

SDesya74 · 2024-07-31T02:43:23Z

src/lib/index.svelte.ts

        delete this.inner[id]
    }

+    edit(message: Message, newContent: string) {


Я имел в виду что вообще метод не нужен, поскольку логика вряд ли поменяется, ведь сообщение - сигнал. Просто подсосемся на его обновление и в будущем будем с бэкендом взаимодействовать

SDesya74 · 2024-07-31T02:44:50Z

src/routes/+page.svelte

    // TODO: Make a multiline chat input that grows dynamically
    let textInputElement: HTMLInputElement | null = $state(null)

+    function saveMessagesOnLocalStorage() {


saveMessagesToLocalStorage

SDesya74 · 2024-07-31T02:45:41Z

src/routes/+page.svelte

+        currentEditingMessage = null
+        textInput = ''
+
+        setTimeout(() => textInputElement?.focus(), 0)


Лучше поменять на tick, в svelte есть

SDesya74 · 2024-07-31T02:46:31Z

src/routes/+page.svelte

+        textInputElement?.focus()
+    }
+
+    function startEditByShortcut(e: KeyboardEvent) {


startEditingByShortcut, как в функции выше

SDesya74 · 2024-07-31T02:49:29Z

src/routes/+page.svelte


    <footer
-        class="dark:text-white bg-slate-100 dark:bg-slate-700 border-t border-gray-300 dark:border-slate-600 p-4 absolute left-0 bottom-0 w-full"
+        class:bg-orange-100={currentEditingMessage}


Необязательно описывать оба варианта, можно в class описать ситуацию по умолчанию, когда !currentEditingMessage, а потом под ним описать дополнение с class:bg-orange-100. Оранжевый может оверрайдить серый, а приоритет у него будет выше, поскольку он указывается после дефолтного. В такой реализации пропадёт дублирование проверки currentEditingMessage

Я изначально так и сделал, это не сработало

Added editMessage function

2cb55f3

JaBrik228 requested a review from SDesya74 July 30, 2024 18:33

SDesya74 requested changes Jul 30, 2024

View reviewed changes

fixed some issues

7c195fb

SDesya74 requested changes Jul 31, 2024

View reviewed changes

Conversation

JaBrik228 commented Jul 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaBrik228 commented Jul 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants