Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PROMO-251(OG): WIP: Add text-overflow for title #410
base: master
Are you sure you want to change the base?
PROMO-251(OG): WIP: Add text-overflow for title #410
Changes from all commits
9bf4e65
886495a
d3f8bc6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nitpick:
Кажется, можно попроще 🤔Мб @Postamentovich еще что подскажет
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.
Прикольно, посмотрю!
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.
nitpick(non-blocking):
Некрит, но тут кажется можно было бы объектом-мапой обойтись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.
угу
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.
suggestion(non-blocking):
Мб что-то типаfunction escapeText
?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.
непонятно звучит конечно, но давай escapeText! =)
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.
Ну тип "экранирование" ~ "чистка"
@GhostMayor Как бы такое назвал?)
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.
Тут надо наоборот делить =)
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.
А поч щас не пофиксить тогда? 🤔
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.
nitpick:
Раз уж речь не про символьную длину, то мб переименовать вmaxWidth
? На манер с cssthoughts:
Тож самое и с паддингом можно сделать кмк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.
хорошая идея.
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.
issue(critical):
ОЧ сложно читаемый и поддерживаемый код кмк(хоть и используется только для постбилда)
suggestion(critical):
Кмк, лучше бы переписать это на манер, как функцияSVGText
- т.е. чтобы сложные значения предпросчитать заранее, а на выход отдавать template-string понятный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.
Щас постараюсь локально еще подсказать как можно кмк упростить
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.
nitpick:
Некрит, но лучше при разбиении отдельно константуwords
завести. Почище будет, и можно к ней обращаться отдельно в цикле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.
nitpick:
Отдельно бы указал, что речь про слова, по которым итерируешьсяТ.к. вне цикла не понятно, что за
cur
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.
issue:
А вот это дело прям упростить надо... Т.к. оч сложно считывается, и прям велика вероятность потом где-то тут ошибку сделатьsuggestion:
Как варианты, что в голову приходят:lines
(опять же по критерию< widthLimit
)2.A. (в идеале) Переписать логику стилей так, чтобы каждый объект можно было обернуть спокойно в
SVGText(...)
и не шаманить в циклах2.B. Если же так нельзя (а учитывая наши замашки по шаблону - так рили может быть) - то хотя бы привести к простому циклу, мб тому же редьюсу - где в константу
contentSvg
редьюсится итоговое значение с учетом переноса строкcontentSvg
,font.declaration
- и гораздо чище получится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.
На самом деле, оч смущает столько хелперов и этапов шаманств с текстом, но учитывая изначальные требования - мб по другому и никак... @feature-sliced/contributors @feature-sliced/core мб будут мнения как еще проще в svgшке текст перенести?
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.
На самом деле я не представляю как ты тут это всё разбирал, магию редьюсов и тд, т.к. это всё в качестве черного варианта было накидано, чтоб вообще расмотреть идею с самописным svg. Кстати owerflow через foreignObject не предлагать, он его не понимает =((
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.
nitpick:
Если консолим, то хотя бы уточни что именно)Особенно если ошибка (хотя непонятно почему ошибка именно должна быть, если тут все валидно)
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.
nitpick:
Лучше сразу обработать негативный сценарий и выбросить значение из функцииА оставшееся тело функции куда будет проще и ацикломатически, и в плане читаемости
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.
угу