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

show link to pool in history comments #634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

show link to pool in history comments #634

wants to merge 1 commit into from

Conversation

akmetainfo
Copy link
Contributor

Это pull request к issue #633

@akmetainfo
Copy link
Contributor Author

Текущая реализация выглядит так:

link1

@madfriend
Copy link
Member

Ссылкой должно быть "#XXXX", не вижу смысла делать ссылкой весь комментарий. Искать #XXXX в комментариях и заменять лучше с помощью preg_replace.

@akmetainfo
Copy link
Contributor Author

Саша, в этом есть своя логика.

Если есть возможность обойтись без регулярок -- то и не надо их использовать, они "тяжелее".

Насчёт ссылки на весь комментарий -- я первоначально хотел ссылку вешать на часть комментария, но более громоздкий код получился. А различие чисто вкусовое.

Подправил немного код.

@madfriend
Copy link
Member

Если не хотите использовать регулярки, используйте хотя бы strpos+substr[_replace], а не substr с фиксированным параметром.

@akmetainfo
Copy link
Contributor Author

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

links2

Обновил реквест.

@akmetainfo
Copy link
Contributor Author

Внёс # внутрь ссылки, скриншот уже не стал менять - а пулл риквест обновил.

@madfriend
Copy link
Member

Чуть позже будем мерджить эту ветку, а то не хочется мастер мерджить в два других пулл реквеста

@akmetainfo
Copy link
Contributor Author

Т.е. поправлять больше ничего не надо, просто ждём удобного момента для того, чтобы слить эту фичу в ствол?

@madfriend
Copy link
Member

да, если Дима одобрит по возвращении.

@grandsbor
Copy link
Member

Честно говоря, мне не нравится ни имя функции history_get_comment_html, ни то, что html-код генерится внутри php-кода, а не в шаблоне. Но я, в принципе, переживу, а если буду страдать, сам потом поправлю.

@madfriend
Copy link
Member

Не надо страдать, лучше до мерджа PR сделать все максимально хорошо, а не доделывать потом :)

Андрей, а почему бы логику замены #ID на ссылку не реализовать в шаблоне templates/history.tpl? А в php-скриптах никаких изменений не делать.
Вот тут: https://williamjxj.wordpress.com/2011/10/28/smarty-preg_match/ пишут, что preg_match работает в шаблонах. Наверное, preg_replace тоже.

@akmetainfo
Copy link
Contributor Author

Андрей, а почему бы логику замены #ID на ссылку не реализовать в шаблоне templates/history.tpl?

А разве против? ) Просто когда я писал письмо и предложил обсудить фичу -- разговор как-то быстро заглох "тут и обсуждать нечего, надо сделать и всё". Не обсудили концепт -- я делал по видению, который был у меня самого.

Могу объяснить, почему я сделал именно так. Если не считать моего некоторого перекоса делать всё на серверной стороне, то надо понимать, что у меня идеальный вариант находился вот в каком направлении: в комментариях разрешен html, вся разметка находится в базе, php тупо достаёт данные из поля, бекенд тупо отрисовывает. Никаких If'ов и функций вообще не планировалось.

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

И хотя и кратко -- но это всё было мной описано изначально, возможно тогда на это не обратили внимания, а всплыло только сейчас?

Всё, что можно поправить до слияния в ствол -- всё нужно править, давайте обсуждать. И имя функции и чёткое разделение на сбор данных и отображение данных. И вливанием в ствол работа над фичей не ограничится, для это был один из промежуточных коммитов (коммит два -- переделка комментариев на html, шаг три -- сделать отдельный таб для ссылок на задания в ещё не до конца промодерированных пулах). Я просто очень не люблю ждать, поэтому для меня всегда проще договориться о маленькой фиче, быстро выкатить чтобы уже ей можно было пользоваться и тут же начать доводить её дальше. А вообще -- сроки выкатывания фич в продакшн у нас очень быстрые: вот один патч, за которым я посматриваю, до сих пор не влили в транк.

@irinfox
Copy link
Member

irinfox commented May 14, 2015

Извините, что влезаю, но если Вы будете писать меньше текста, то нам будет
легче Вам отвечать :)
Когда видишь много букв, первая реакция - закрыть письмо и уйти спокойно
работать.
Мы все работаем.

Ира

14 мая 2015 г., 11:34 пользователь Andrey [email protected]
написал:

Андрей, а почему бы логику замены #ID на ссылку не реализовать в шаблоне
templates/history.tpl?

А разве против? ) Просто когда я писал письмо и предложил обсудить фичу --
разговор как-то быстро заглох "тут и обсуждать нечего, надо сделать и всё".
Не обсудили концепт -- я делал по видению, который был у меня самого.

Могу объяснить, почему я сделал именно так. Если не считать моего
некоторого перекоса делать всё на серверной стороне, то надо понимать, что
у меня идеальный вариант находился вот в каком направлении: в комментариях
разрешен html, вся разметка находится в базе, php тупо достаёт данные из
поля, бекенд тупо отрисовывает. Никаких If'ов и функций вообще не
планировалось.

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

И хотя и кратко -- но это всё было мной описано
https://github.com/akmetainfo/opencorpora/issues/13 изначально,
возможно тогда на это не обратили внимания, а всплыло только сейчас?

Всё, что можно поправить до слияния в ствол -- всё нужно править, давайте
обсуждать. И имя функции и чёткое разделение на сбор данных и отображение
данных. И вливанием в ствол работа над фичей не ограничится, для это был
один из промежуточных коммитов (коммит два -- переделка комментариев на
html, шаг три -- сделать отдельный таб для ссылок на задания в ещё не до
конца промодерированных пулах). Я просто очень не люблю ждать, поэтому для
меня всегда проще договориться о маленькой фиче, быстро выкатить чтобы уже
ей можно было пользоваться и тут же начать доводить её дальше. А вообще --
сроки выкатывания фич в продакшн у нас очень быстрые: вот один патч
http://habrahabr.ru/post/238303/, за которым я посматриваю, до сих пор
не влили в транк.


Reply to this email directly or view it on GitHub
#634 (comment)
.

Ирина

@akmetainfo
Copy link
Contributor Author

@irinfox И вы меня извините, что так длинно пишу.

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