Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Добавляет кнопку «загрузить картинку» #1221

Merged
merged 13 commits into from
Jul 19, 2024

Conversation

nlopin
Copy link
Collaborator

@nlopin nlopin commented Jun 28, 2024

Что

Кнопка загрузки картинок для тех, кто не может перетаскивать картинки на форму 🥳

Demo

Как

  1. дописал дополнительный обработчик в код загрузки картинок (frontend/static/js/inline-attachment.js)
  2. к каждому редактору создаётся контрол выбора файла (<input type="file" />)
  3. изменена верстка всех ответов. Кнопка «отправить» теперь визуально часть поля, в котором пишется ответ

До и после

Поле До После
Комментарий CleanShot 2024-06-28 at 23 32 12@2x CleanShot 2024-06-28 at 23 34 44@2x
Ответ CleanShot 2024-06-28 at 23 35 12@2x CleanShot 2024-06-28 at 23 35 28@2x
Ответ (узкий экран) CleanShot 2024-06-28 at 23 36 32@2x CleanShot 2024-06-28 at 23 36 09@2x
Баттл CleanShot 2024-06-28 at 23 38 46@2x CleanShot 2024-06-28 at 23 37 14@2x
Интро CleanShot 2024-06-28 at 23 39 16@2x CleanShot 2024-06-28 at 23 39 36@2x

@nlopin nlopin requested a review from vas3k as a code owner June 28, 2024 21:39
@nlopin nlopin force-pushed the feat/add-attach-file-button branch from 59faef5 to d14812a Compare June 28, 2024 21:46
@trin4ik
Copy link
Contributor

trin4ik commented Jun 29, 2024

Кайф! Я бы сделал следующее:

  • иконка скрепки ужасная, может всё же камеру? смотрится не плохо

image

  • сделать display: none для инпута и обернуть его в label, которому сделать cursor: pointer т.е. типа того
<div class="reply-form-footer">
    <label class="reply-form-attach-image">
        <i class="fa fa-paperclip"></i>
        <input type="file" alt="Добавить картинку" />
    </label>
</div>
.reply-form-attach-image {
  ...
  cursor: pointer;
}
.reply-form-attach-image input[type=file] {
  ...
  display: none;
}
  • "подписаться на комментарии" длинновато для мобилки, на правах "мысли вслух", может определим какю-то глобальную переменную во вью, чтобы где надо проверять на isMobile и подобные "подписаться на комментарии" менять на "подписаться"?

@TiraelSedai
Copy link
Contributor

на мой взгляд камера ещё хуже скрепки
image
вот эту иконку бы экспроприировать, но тогда "старую" надо или убирать или заменять

у гитхаба это тоже синонимичные кнопки
image

@nlopin
Copy link
Collaborator Author

nlopin commented Jun 30, 2024

  • сделать display: none для инпута и обернуть его в label, которому сделать cursor: pointer т.е. типа того

в чем будет отличие от того, что сейчас?

@trin4ik
Copy link
Contributor

trin4ik commented Jun 30, 2024

  • сделать display: none для инпута и обернуть его в label, которому сделать cursor: pointer т.е. типа того

в чем будет отличие от того, что сейчас?

было стало
before2 after2

@nlopin
Copy link
Collaborator Author

nlopin commented Jun 30, 2024

класс, у меня такого нет. Поправлю верстку 🤝 Что это за браузер, чтобы я мог проверить?

@trin4ik
Copy link
Contributor

trin4ik commented Jul 1, 2024

класс, у меня такого нет. Поправлю верстку 🤝 Что это за браузер, чтобы я мог проверить?

хром в убунте, современный. посмотрю в других браузерах линя, отпишу

@trin4ik
Copy link
Contributor

trin4ik commented Jul 1, 2024

класс, у меня такого нет. Поправлю верстку 🤝 Что это за браузер, чтобы я мог проверить?

ubuntu 22.04
в 127 фаерфоксе всё ок, как и должно быть
в 126 хроме тот баг, что я описал, т.е. иконка курсора работает неверно.

@vas3k
Copy link
Owner

vas3k commented Jul 1, 2024

Скажите когда можно будет тестить :) У нас по понеделькам деплоить нельзя, так что у вас еще есть время

@nlopin
Copy link
Collaborator Author

nlopin commented Jul 1, 2024

класс, у меня такого нет. Поправлю верстку 🤝 Что это за браузер, чтобы я мог проверить?

ubuntu 22.04 в 127 фаерфоксе всё ок, как и должно быть в 126 хроме тот баг, что я описал, т.е. иконка курсора работает неверно.

Попробуй новую версию, у меня в хроме заработало. Это микс твоего предложения с предыдущей версией. Мы не можем сделать display:none потому что перестанет работать фокус через Tab.

@nlopin
Copy link
Collaborator Author

nlopin commented Jul 1, 2024

Скажите когда можно будет тестить :) У нас по понеделькам деплоить нельзя, так что у вас еще есть время

можно

@trin4ik
Copy link
Contributor

trin4ik commented Jul 1, 2024

класс, у меня такого нет. Поправлю верстку 🤝 Что это за браузер, чтобы я мог проверить?

ubuntu 22.04 в 127 фаерфоксе всё ок, как и должно быть в 126 хроме тот баг, что я описал, т.е. иконка курсора работает неверно.

Попробуй новую версию, у меня в хроме заработало. Это микс твоего предложения с предыдущей версией. Мы не можем сделать display:none потому что перестанет работать фокус через Tab.

ага, всё норм

@nlopin
Copy link
Collaborator Author

nlopin commented Jul 3, 2024

@vas3k что скажешь?

@nlopin
Copy link
Collaborator Author

nlopin commented Jul 18, 2024

@vas3k давай снова тестировать.

Изменения:

  • иконка скрепки стала картинкой (fa-image)
  • загрузка для мобилы работает с простой textarea
  • полноценный редактор теперь не инициализируется на мобиле

Пришлось вытащить часть логики из inline-attachment, чтобы не дублировать. Чуть позже хорошо бы этот файл переписать на более современный код, чтобы уйти от IIFE в нем. Это было явно сделано для того, чтобы подключать файл напрямую в браузер и для нас неактуально, мы его бандлим

@vas3k vas3k merged commit 4485776 into vas3k:master Jul 19, 2024
6 checks passed
@nlopin nlopin deleted the feat/add-attach-file-button branch July 19, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants