-
Notifications
You must be signed in to change notification settings - Fork 65
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
Рефакторит toc-text-crop.js
#1041
Conversation
Круто! Спасибо за оптимизацию! А что будет, если фраза будет что-то вроде «Фильдеперсовый Константинопольский шпалоукладчик звукоизвлекает сложносочинённые турбопропизоляционные ноктюрны»? Кажется, в этом случае в боковом меню будет слишком много текста и он даже может вывалиться за пределы контейнера. Смысл работы с символами был именно в том, чтобы контролировать длину подзаголовка без оглядки на длину слов. |
Получится как-то так: Но если серьезно, интернет говорит, что средняя длина слова в русском языке до 8-9 букв (деловая и научная литература). Я ради интереса вбил все вопросы из этого пиара и посчитал среднюю длину слова: Я это всё к тому, что такие длинные слова в языке это что-то необычное, не уверен, что их можно принимать во внимание, т.к. могут ли они вообще встретиться? Вижу такие варианты:
|
Если важно считать именно символы, может, такой вариант имеет место быть? То есть мы делаем массив и в цикле удаляем слова, пока длина не придёт в норму. Плюс по сравнению с текущим вариантом в том, что мы используем очищенные от лишних пробелов данные, ну и если вдруг появятся Минус в том, что второго цикла избежать не удаётся. |
Привет! Мне нравится вариант с подсчётом символов, предлагаю на нём и остановиться. А давай ещё напишем тест на всё это? У нас новый тренд на 100% покрытие платформы тестами. Надо с чего-то начинать. |
Тест сделаю с удовольствием) |
Добавил тест, но возникла небольшая проблема. Чтобы проверить, как функция работает с DOM, добавил jsdom. После этого всё работает, но смущает, норм ли такая история в рамках Доки вообще?) Не намудрил ли я лишнего? Обсуждение, почему для jsdom нужно столько манипуляций: inrupt/solid-client-authn-js#1676 |
Кажется, так должно быть лучше. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что касается правильного алгоритма... Может мы просто обрежем регуляркой максимум сколько-нибудь символов и потом то что получилось обрежем еще до первого пробела?
* @jest-environment jsdom | ||
*/ | ||
|
||
const { JSDOM } = require('jsdom') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я думаю добавить jsdom хорошая идея. Но раз уж мы его добавляем, давай делать это везде.
Давай перенесем это в jest.setup.js
?
…s/platform into toc-text-crop/refactor
…s/platform into toc-text-crop/refactor
Да, печальная картина. Моя ветка безнадежно отстала от темпов современного мира, но скромная попытка ее обновить привела к неожиданному приклеиванию свежих комитов и дублированию моих. Похоже, единственный путь для странника — начать всё по новой. Пересоздам заново? |
Привет!
Предлагаю чуть-чуть протюнить прекрасную функцию clipContent. Два главных апгрейда:
Решение:
Считать сразу слова, а не символы. Выбрал максимум в 10 слов.
for
лишний. С массивомstringArray
уже сразу можно работать, а сейчас создаётся дополнительная конструкция-строкаtmpString
, с которой и происходят главные манипуляции.Решение:
Убрать лишний цикл. Сократил файл на 7 строк.