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

Preact Version #4

Merged
merged 44 commits into from
Nov 17, 2018
Merged

Preact Version #4

merged 44 commits into from
Nov 17, 2018

Conversation

Reeywhaar
Copy link
Collaborator

Чуть-чуть обновленная версия на preact

Тестовая версия крутится здесь: http://rtnews.vyrtsev.com/

Изменения

  • так как комментарии теперь на remark42, то нет сортировки "по лайкам"
  • есть страница с сортировкой новостей
  • чтобы добавить новость, теперь можно дропнуть ссылку в главное окно
  • возможность автоматического скролла к текущей теме

Возможны какие-то баги, или недопонимание. Я, например, так и не понял предназначение кнопки "поехали" до конца.

@igoradamenko
Copy link
Collaborator

Привет!

Лайк за идею. Я больше года собирался это сделать, но всё не находил времени :-)

Я, например, так и не понял предназначение кнопки "поехали" до конца.

Она отправляет на сервер инфу о том, что выпуск начался, чтобы корректно синхронизировались таймкоды, когда ведущие жмут «Сделать текущей».

Смотри, что сейчас смущает:

  1. На news.radio-t.com индикатор загрузки сейчас «Загружаю», и он аккуратный и по центру. А у тебя сейчас «Loading», и он какой-то большой и странный.

  2. «Свежие» и «Все» не выглядят как кнопки, и при фокусе ещё не очень красиво выглядят.

  3. Ошибка 404 с «Go to main page» тоже странно выглядит. Всё ж на русском :-)

  4. Мало где есть ховеры, и почти нигде, кроме ссылок, нет cursor: pointer. Это раздражает.

  5. Тот факт, что картинка может сместить всю новость — не круто. Кажется, в оригинальной версии иначе работало (смещало только то, что нужно было сместить).

image

  1. Что делает кружок рядом с «К текущей теме», не понял :-)

  2. Прописанные width / height вкупе с max-width: 100% не очень хорошо работают:

screenshot_20181105-120845

  1. В контенте некоторых страниц возможно вот такое. Надо бы это предусмотреть и добавить скролл для pre / code и пр.:

screenshot_20181105-121418

  1. Зашёл на страницу новости с мобильного, нажал «К комментариям», оно прокрутило, но не до конца:

screenshot_20181105-121430

  1. Кнопки сортировки в мобильной раскладке прыгают, при нажатии на них (из-за того, что где-то исчезает бордер, а где-то что-то становится жирным):

image

  1. Раньше при нажатии на «Подробнее» новость показывалась прямо тут, на месте. А сейчас она просто переводит на страницу новости.

Про админку:

  1. «via» в новостях лишнее, потому что чаще всего просто дублирует домен.

  2. То, что можно дропнуть ссылку, это прикольно, но кажется, что кнопка «Добавить новость» теперь слишком незаметная.

  3. Пропала кнопка «Сделать первой» (она появляется на всех, кроме первой новости).

  4. Пропал ультра-скилл сортировки. Сейчас, если в админке навести на правую часть новости, то можно увидеть, что курсор меняется на перетаскивающий, потому что там можно потянуть новость, и тем самым поменять их порядок. Но вижу, что теперь есть отдельная вкладка с «Сортировать новости». Не знаю, насколько это удобно.

  5. Раньше в админке гиковские новости выделялись синей рамкой, потому что, кажется, так было удобнее. Она более заметна, и когда сортируешь новости к гиковскому выпуску, получается быстрее сориентироваться. Текущий кружок возле тайтла, кажется, не заменяет этого.

Это то, что удалось сходу заметить. Не знаю, насколько всё это критично, и насколько важно. Тут стоит дождаться мнения @umputun, потому что так или иначе ему этим пользоваться.

По коду попозже посмотрю 🐨

@Reeywhaar
Copy link
Collaborator Author

@igoradamenko привет! Спасибо за толковые комментарии, и тщательный подход к дизайну. Мне в принципе твоя текущая версия очень нравится, как видишь, изменения минимальны, вообще я просто хотел поменять комментарии на ремарк, но тут пошло-поехало.

По пунктам, где есть замечания:

  1. Согласен, хорошо
  2. Согласен, хорошо
  3. Согласен, хорошо
  4. Про пойнтеры это старый холивар. Я раньше тоже везде пальчик ставил, а потом посмотрел на окно файндера, и перешел в другой лагерь :-) Тут, наверное, как скажет @umputun . Только в месте фильтров и сортировки, да, надо добавить какой-то ховер.
  5. Хм. да, это какой-то баг. Это же хром на андроиде? У меня к сожалению нет андроида, я могу только попросить одолжить потестить.
  6. Там есть title: в активном состоянии, при смене текущей темы на главном окне, происходит автоматический скролл к ней.
  7. Это надо поправить.
  8. Я вроде добавил скролл для контента. Это наверное что-то на андроиде, надо разобраться.
  9. Надо поставить таймаут до загрузки страницы. Это из-за загружающихся картинок, я так понимаю.
  10. Согласен.
  11. Удивительно, сколько я не смотрел на этот сайт, никогда не замечал! Надо сделать.

Про админку:

  1. То что via дублирует, это частный случай тестового сервера. В жизни, я знаю что есть две подписки c instapaper.com и это еще один фактор, который может помочь с сортировкой тем. То есть если тема добавлена, например вручную, то via не будет, а значит добавил кто-то из ведущих и надо обратить на тему внимание. via не показывается рядовым пользователям.
  2. Возможно. Там, кстати, есть старый роут на /add/ который работает как прежде, если у кого-то сохранена закладка. Опять же, пусть @umputun рассудит.
  3. Эта кнопка есть на главной странице, если сброшены все фильтры и стоит сортировка по приоритету, также как на текущей версии.
  4. Опять же, только сейчас узнал, что есть такая вещь! Я думал есть только кнопка сделать первой. Я теперь на распутье, с одной стороны на странице сортировки темы без контента, сжатые горизонтально, что позволяет взглянуть взглядом орла и перетащить через большее количество тем, особенно на сафари, который при драге к краю экрана не начинает скроллить. С другой стороны старый интерфейс тоже очень хорош, и даже более отзывчив. Мой вариант ждет когда новость сохранится, чтобы избежать конфликтов.
  5. Тут да, надо послушать @umputun . Мне кажется рамка ту мач.

Еще раз спасибо за замечания!

@umputun
Copy link
Member

umputun commented Nov 5, 2018

В качестве общего замечания - практически вся UI функциональность "старой" версии полезна и используется.

По тому, что заметил в новой версии сразу:

  1. не получается никак поменять порядок тем претаскиванием. Эти супер важная фича.
  2. а, нашел на отдельной вкладке. Не понятно почему так из зачем, это точно меннее удобно для пользования чем раньше.
  3. "подробнее" открываться должно без перехода. Тоже, черезвычайно полезно
  4. Выбор темы стал какой-то тормознутый, "активирую" висит несколько секунд
  5. про драг-дроп для новой темы интересно, хотя не очень понятно как оно будет использоваться на практике
  6. "Добавить новость" не ставит фокус на поле ввода URL
  7. рамка или кружочек - в принципе оба хорошо, можно и кружочек
  8. залогинится в ремарк я не смог. Видимо ты используешь новый siteID (какой?) - мне его надо добавить на своей стороне
  9. при перетаскивании непонятно где тема окажется, нет визуальной "метки", раньше было понятнее
  10. сортировки (по приоритету, по дате .... и т.п) ничего не делают

Все это было попробовано на маке, chrome

@umputun
Copy link
Member

umputun commented Nov 5, 2018

в догонку:

  • кружочек в "к текущей теме" мне непонятен, что он вообще должен делать? И попасть в него трудно
  • я люблю неконтрастные сайты, но тут читать тяжело, слишком светлый серый цвет
  • "via ..." совсем лишнее. Если цель была отделить новости добавленные вручную таким образом, то это слишком неочевидно

@Reeywhaar
Copy link
Collaborator Author

  • про сортировку понял, надо улучшать.
  • про "подробнее" тоже
  • выбор текущей темы ждет ответа от сервера, он почему-то долгий. Тут да, надо работать над фидбеком в общем. Сейчас все действия происходят после ответа с сервера, что замедляет интерфейс
  • про ремарк: кстати да, забыл сказать, нужен какой-то конфиг к нему. Сейчас там просто site_id: "radiot" и url текущей страницы.
  • то что сортировки ничего не меняют: да, это какой-то баг в хроме, надо посмотреть.
  • про кружочек: если он в активном состоянии, то при смене текущей темы произойдет автоскролл к ней. Чтобы понять можно открыть в разных окнах, одно проскроллить до конца, и выбрать самую нижнюю тему текущей, а потом перейти в другое окно и ждать. Просто когда вы сами меняете тему, то она и так в окне. Это наверное полезно другим участникам. На текущей версии при смене темы меняется тайтл и выскакивает нотификейшн, что тема сменилась.
  • про неконтрастность не понял, можно пример? Я ничего не делал с цветами, текст черный, фон белый.
  • via тогда уберу.

@umputun
Copy link
Member

umputun commented Nov 5, 2018

про ремарк: кстати да, забыл сказать, нужен какой-то конфиг к нему. Сейчас там просто site_id: "radiot" и url текущей страницы.

Что-то там не так тогда. С radiot id я должен был бы зайти без проблем. Но по любому, это плохая идея смешивать сайт и новости в одном id, добавил rtnews

про неконтрастность не понял, можно пример?

было:

zk4ts-201811-05122834-oc771

стало:

9vcew-201811-05122847-b8qqm

На скришотах не очень заметно, но сели открыть side-by-side то разница весьма ощутимая. Может не цвет, но фонт, трудно сказать

Про долгий ответ - да, с моей стороны тормоза. это от тестовой версии, оно там падает в попытке запосить на гиттер новую тему

rt-news-dev | 2018/11/05 12:31:45 [INFO]  activate id=5be0699cd1ce131acedcb32d
rt-news-dev | 2018/11/05 12:31:45 [INFO]  activate event, Ломаем фундаментальные основы C#: выделение памяти под ссылочный тип на стеке, https://habr.com/post/428676/?utm_source=habrahabr, [172], id=5be0699cd1ce131acedcb32d
rt-news-dev | 2018/11/05 12:31:45 [INFO]  update active article via news.radio-t.com:18001
rt-news-dev | 2018/11/05 12:31:50 [WARN]  failed to connect to news.radio-t.com:18001, error=dial tcp 46.101.99.10:18001: i/o timeout
rt-news-dev | 2018/11/05 12:31:50 [WARN]  failed to update article lomaem-fundamental-nye-osnovy-c-vydelenie-pamiati-pod-ssylochnyi-tip-na-steke, dial tcp 46.101.99.10:18001: i/o timeout

fix add-form autofocus
fix 404 and loading
adjust remark config
add article detailed snippet
fix chrome sorting
remove feed labels
adjust contrast
optimize article data
fix to-comments scroll
avoid possible img aspect distortion
add drop indicator
add drag'n'drop feature on main page
night theme feature
fix theme switch
refine notifications and article activation
improve drag'n'drop

reduced server load

news autoupdate

prerendered html

adjust theme switching

adjust notifications

refine article polling

scroll to detailed link on detailed view side close

collapse article when dragging

hint react production build

bolder drag border indicator

active article notification auto-remove

notification on active article not found

comb imports

refactoring
@Reeywhaar
Copy link
Collaborator Author

Хм, опять с гитхабом что-то творится, прям беда. Добавил коммит а он здесь не показывается.
В общем живая версия все также на http://rtnews.vyrtsev.com/

Исправления

  • "Загружаю" по центру
  • 404 страница на русском
  • Картинка с floatом не сдвигает контент
  • Надеюсь поправил ситуацию с img и height. Поправил вслепую, так как не нашел в каком это было посте.
  • Контент теперь не должен раздвигать страницу
  • Добавил таймаут при переходе к комментариям, чтобы успели прогрузиться картинки, но все равно, в реальном мире могут быть ситуации когда картинка появляется и через 3 и через пять секунд. С обычными anchor ссылками такая же ситуация
  • Кнопки сортировки больше похожи на старые и не прыгуют
  • "Подробнее" работает
  • via убрал
  • Ультра скилл сортировки теперь и на главной странице, но работает он как и в текущей версии, только если сняты все фильтры и стоит сортировка "по приоритету"
  • При отрытии "добавить новость" фокус стоит на поле с урлом
  • Я посмотрел, у меня ремарк работает, я даже оставил несколько комментариев, правда для урла localhost:9000/news/что-то-там
  • Сортировки в хроме исправил
  • Вроде поправил контраст (Да, у меня на мониторе еле-еле заметна разница, он у меня темные съедает)
  • Фидбек в целом намного быстрее, и меньше нагрузка на сервер (список тем теперь не обновляется при удалении, архивировании, восстановлении, перетаскивании и проч.)
  • Добавил переключатель дневная/ночная тема (это вообще нормально?)

В общем, пинг

@umputun
Copy link
Member

umputun commented Nov 7, 2018

из того, что сразу кинулось в глаза:

  • login в ремарк я так и не смог произвести. @igoradamenko посмотришь что там не так? Логин оно вроде делает и редирект тоже, но что-то явно не так
  • при перетаскивании тем выше/ниже границ экрана, скрол тормозит и дергается
  • две не очень понятные иконки и обе в виде кружочков, может подобрать как-то со смыслом? И еще один для гиковских тем.
  • в темной теме черный уж сильно черный
  • непонятно зачем вообще view "сортировать темы"

Все это на сафари

@Reeywhaar
Copy link
Collaborator Author

Reeywhaar commented Nov 7, 2018

  • Действительно, в сафари не работает вход в ремарк. В хроме и фаерфоксе все ок. Если в сафари снять галку с "prevent cross-site tracking" то начинает работать. Непонятно, это проблема rtnews или ремарка? Там я знаю уже есть проблема с тем, что сортировки в ремарке сбрасываются. Еще может быть проблема с тем что сайт сейчас на http, может из-за отсутствия ssl какие-то дополнительные предосторожности, на radio-t.com же работает.
  • Надо посмотреть как в текущей версии скролл в сафари организован.
  • Буду благодарен @igoradamenko если он что-то придумает для иконок, если ему интересно
  • Боялся, что темный будет неконтрастным, надо взять пример с какого-нибудь monokai
  • Страница с сортировкой мне кажется удобна тем (хотя сейчас она уже полностью дублирует главную), что там все более компактно и позволяет быстрее менеджить темы, особенно на маленьких экранах. Я думал будет удобно.

@umputun
Copy link
Member

umputun commented Nov 7, 2018

Если в сафари снять галку с "prevent cross-site tracking" то начинает работать. Непонятно, это проблема rtnews или ремарка?

Подозреваю, что когда они будут на одном домене (blah.radio-t.com) то будет работать.

Страница с сортировкой мне кажется удобна тем (хотя сейчас она уже полностью дублирует главную), что там все более компактно и позволяет быстрее менеджить темы

Я не против, просто не понял зачем это. В принципе есть смысл

Еще одно - в текущей версии на мобильном, где перетаскивать нереально, появляются кнопки для движения новостей вверх/вниз. Тут я такого не увидел переключив на сафари агент в ios

@Reeywhaar
Copy link
Collaborator Author

Reeywhaar commented Nov 7, 2018

Подозреваю, что когда они будут на одном домене (blah.radio-t.com) то будет работать.

Это хорошо, но все равно получается у ремарка есть проблема у других людей на других сайтах.

Да, на мобильном/айпаде надо подумать. Может можно и хороший драг/дроп сделать.

safari drag scrolling smooth

less black night mode

make article content placement relative (to avoid absolutely positioned elements)

add hash to location on active-post/comment move

fix gitter link color in night mode

add auto-scroll and geek article icons
minor
show 404 on not found article

polling fail proof

try add missing domain in article response

fix follow icon

improved behavior on missing current article

unified scroll behaviour
fix controls flickering

update news while idle

listing controls improved feedback

improved behaviour of header pseudo links

show active article no matter what

adjust loading placeholder

remove autoscroll feature
@Reeywhaar
Copy link
Collaborator Author

Reeywhaar commented Nov 8, 2018

Еще пачка изменений:

  • заработало перетаскивание тем на тач устройствах (проверял на айфоне и в хроме в device mode)
  • поправил скачущий скролл в сафари
  • менее черный бэкграунд в ночной теме
  • убрал иконку автоскролла. Что-то как-то с ней не так.
  • всякие фиксы и улучшение отзывчивости

Еще заметил, что /news/active/id не работает, приходится тянуть полный пост. И еще было бы хорошо чтобы при удалении текущей темы это тоже как-то отражалось в /news/active/wait/:secs, например пустым ответом или "null".

@umputun
Copy link
Member

umputun commented Nov 11, 2018

хмм. я там вижу вообще 500

@Reeywhaar
Copy link
Collaborator Author

Починил, в конфиге nginx стоял хедер add_header Strict-Transport-Security "max-age=604800";
В общем не должно перебрасывать на https, а то https->http блокируется.

@Reeywhaar
Copy link
Collaborator Author

@umputun а по какому url поста бекенд ищет количество комментариев? Я написал с разных хостов когда тестировал, но все это мимо (если на дату комментария навести видно с каких хостов).

@umputun
Copy link
Member

umputun commented Nov 13, 2018

вот этот годный https://news.radio-t.com/post/the-sharashka-phenomenon-2011

там у меня была опечатка в коде, поправил, теперь должно матчить верно

@umputun
Copy link
Member

umputun commented Nov 15, 2018

какой статус этого PR? Оно готово/в работе/ждет review от @igoradamenko ?

Какой статус того, что на http://rtnews.vyrtsev.com, оно последнее? Если да, то управление фидами полмалось, там пустой список

@Reeywhaar
Copy link
Collaborator Author

На тестовой версии, я просто удалил все фиды, чтобы зря не качало, или не добавляется фид? Да, там свежее.

Жду ревью да, @umputun @igoradamenko говорите.

@umputun
Copy link
Member

umputun commented Nov 15, 2018

или не добавляется фид

Все в порядке, добавляется. Я просто не ожидал что они были удалены. Кстати, в фидах хорошо бы фокус на поле ввода ставить.

@Reeywhaar
Copy link
Collaborator Author

Поправил автофокус + несколько улучшений.

@umputun
Copy link
Member

umputun commented Nov 15, 2018

Я не очень помню, что "свежие" должно было ограничивать, но на вид оно ничего сейчас не меняет

@Reeywhaar
Copy link
Collaborator Author

Reeywhaar commented Nov 15, 2018

Я скопировал из старой версии, там было так:

fn(x, i, isgeek = true) {
	const interval = now - x.parsedats;
	if (isgeek && x.geek && interval < 3 * month) {
		return true;
	} else if (interval < 21 * day) {
		return true;
	}
	return false;
}

Если переключатель с темами, там где "все/обычные/гиковские" стоит на "гиковские", то "свежие" означает темы не старше чем три месяца, если стоит "обычные" или "все" значит не старше чем 21 день. Я проверял, работает также как на текущей версии, просто на тестовом сервере все новости добавлены недавно.

@@ -0,0 +1,29 @@
<html lang="en">

This comment was marked as off-topic.

)
.join("&");

node.innerHTML = `

This comment was marked as off-topic.

@igoradamenko
Copy link
Collaborator

igoradamenko commented Nov 16, 2018

Меня смущает, что во всём этом нет системы или какого-то компонентного подхода. БЭМ там, или что-нибудь вроде того. Все классы лежат рядом плоским списком, и это странно и нетипично для фронтенда, как и Makefile, например :-) Ну и там, по мелочи: кавычки не те, табы вместо пробелов, то-сё.

Но было бы странно просить переписать всё так, как мне нравится, если оно изначально совсем не так.

Потому, если @umputun устраивает как оно работает, стоит выкатить в продакшен, собрать багрепортов и дофиксить, если будут.

Единственное, что меня сильно смущает, это то, как подключается Ремарк. @Reeywhaar, так и будет? Выглядит дико.

@Reeywhaar
Copy link
Collaborator Author

Ну цсс можно сказать по БЭМу, кроме М. Такой у меня стиль, не люблю диких вложенностей, мне тоже неудобно ходить по всяким "__comments/_something". Это такое, понимаешь, зачем усложнять :-). Я наоборот старался, чтобы всем кто захочет предложить pr не приходилось вникать в структуру.
Makefile прекрасен :-) Табы прекрасны, кавычки прекрасны, это стандартный prettier без всего.

По поводу ремарка, да, там по сути вшит embed.js, я сделал так специально, чтобы если в embed.js что-то поменяется, то легко можно было бы поменять и здесь. Это кстати хороший issue для ремарка "интеграция для SPA". Сейчас он ищет по id, и если его нет, то падает с "remark root not found", хотя хорошо бы чтобы он отслеживал с помощью mutationObserver например.

По поводу lang=en/ru, я тут даже на знаю, если посмотреть на пропорцию en/ru в новостях, то явно en превалирует. Учитывая, что в основном аттрибут нужен, чтобы правильно ставить переносы в словах, то может логичнее оставить en? Элементы интерфейса все равно без переносов.

@umputun umputun merged commit df99375 into radio-t:master Nov 17, 2018
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.

3 participants