Added editMessage function - #9
Conversation
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0" encoding="iso-8859-1"?> | |||
There was a problem hiding this comment.
Прогони через svg optimizer чтобы убрал комменты и лишнее
| } | ||
|
|
||
| export type Message = { | ||
| export interface IMessage { |
There was a problem hiding this comment.
Не уверен что следует слой логики переименовывать ради фронтенда
| delete this.inner[id] | ||
| } | ||
|
|
||
| edit(id: Id, newContent: string) { |
There was a problem hiding this comment.
А почему бы просто сообщению контент не установить, раз оно в onedit доступно? Реактивное же
| } | ||
|
|
||
| function onEditMessage() { | ||
| if (currEditableMessageId) { |
There was a problem hiding this comment.
Условие можно инвертировать, тогда вложенность будет меньше
if (!currEditableMessageId) {
return
}There was a problem hiding this comment.
И имя currEditableMessageId наверное лучше заменить на currentEditingMessage, возможно даже лучше сделать не Id, потому что message - сигнал и по Id его можно не таскать
| } | ||
| } | ||
|
|
||
| function focusOnMessage(value: IMessage) { |
There was a problem hiding this comment.
function startEditing мб? Более декларативно звучит и объясняет, зачем нужна функция
| textInputElement?.focus() | ||
| } | ||
|
|
||
| function onEditMessageByKeyDown(e: KeyboardEvent) { |
There was a problem hiding this comment.
Та же история, startEditByShortcut или типа того, лучше именовать бизнес функции по смыслу, а не по действию
|
|
||
| <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`} |
There was a problem hiding this comment.
Тут можно воспользоваться свелтовским синтаксисом
class:bg-orange-100={currEditableMessageId}
А, ну и bg-orange возможно стоит поменять на bg-accent с прозрачностью через bg-opacity. Хотя текущая механика установки цветов прозрачность не поддерживает... В целом можно и bg-orange оставить, но если есть желание прямо в этом ПР починить и прозрачность цветов, можешь это сделать. В файле со стилями темы вроде даже написано как
|
Нашёл баг: При изменении сообщения, оно не обновлялось в localStorage. Так же внёс все правки, по твоим замечаниям |
| delete this.inner[id] | ||
| } | ||
|
|
||
| edit(message: Message, newContent: string) { |
There was a problem hiding this comment.
Я имел в виду что вообще метод не нужен, поскольку логика вряд ли поменяется, ведь сообщение - сигнал. Просто подсосемся на его обновление и в будущем будем с бэкендом взаимодействовать
| // TODO: Make a multiline chat input that grows dynamically | ||
| let textInputElement: HTMLInputElement | null = $state(null) | ||
|
|
||
| function saveMessagesOnLocalStorage() { |
| currentEditingMessage = null | ||
| textInput = '' | ||
|
|
||
| setTimeout(() => textInputElement?.focus(), 0) |
There was a problem hiding this comment.
Лучше поменять на tick, в svelte есть
| textInputElement?.focus() | ||
| } | ||
|
|
||
| function startEditByShortcut(e: KeyboardEvent) { |
There was a problem hiding this comment.
startEditingByShortcut, как в функции выше
|
|
||
| <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} |
There was a problem hiding this comment.
Необязательно описывать оба варианта, можно в class описать ситуацию по умолчанию, когда !currentEditingMessage, а потом под ним описать дополнение с class:bg-orange-100. Оранжевый может оверрайдить серый, а приоритет у него будет выше, поскольку он указывается после дефолтного. В такой реализации пропадёт дублирование проверки currentEditingMessage
There was a problem hiding this comment.
Я изначально так и сделал, это не сработало
2 participants
No description provided.