-
Notifications
You must be signed in to change notification settings - Fork 178
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: OpenGraph dynamic image plugin prototype. #368
PROMO-251: OpenGraph dynamic image plugin prototype. #368
Conversation
132de27
to
d287b7b
Compare
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.
- Верни пож ченджлог, который был в темплейте PR)
- Добавь пож комменты к диффу своему, чтобы было проще ревьювить и предлагать изменения
(оч хочется втянуть, но многие вещи не оч интуитивно понятны - лучше бы дополнить их комментарием, например как тут)
Как все сделаешь - пингани еще раз как можно будет ревьювить))
Думаю втянем сразу же, только с небольшими правками, чтобы прод не развалить
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.
Добавил комментарии по PR.
images.set(`${item.name}_${item.params.image}`, createImageFromTemplate(item)); | ||
} | ||
}); | ||
return images; |
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.
Создание мапы уникальных sharp pipeline'ов для наших картинок. (проверка на уникальность пока бесполезна, т.к. в layout кроме текста ничего добавлять пока что нельзя, и у нас по сути нет других картинок в шаблоне).
// TODO: File not found exception? | ||
const fonts = createFontsMapFromTemplates(templates); | ||
const images = createImagesMapFromTemplates(templates); | ||
|
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.
Создаём коллекции шрифтов и картинок, которые в дальнейшем будем клонить и потом с ними работать.
console.error("Config validation error"); | ||
return; | ||
} | ||
return config; |
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.
Чтение и валидация нашего основного конфига плагина.
…not default locales.
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.
Таки посмотрел... Выглядит прям монструозно, крутецкую работу проделал!
Вопросы такие:
-
Можно ли как-то оставить в плагине только критичный функционал, чтоб без проблем затащить что есть?
(а то по многим модулям там есть вопросы, и кажется, что расширять функционал надо бы позднее, а не прям щас - сложно пока даже умом объять. И кажется именно из-за сильно огромного функционала влитие PR может затянуться. Если можно оставить только базис - я бы оставил действительно только его на данный момент .А остальное след. итерациями) -
Можно ли локально при билде - получить OG изображения файлов?
(как я понимаю - изображения сгенерятся при билде, и потом по хешу, что будет вhead.meta.og:image
можно будет перейти по ссылке и получить статическое превью - верно?)
const layers = layout.map((item) => { | ||
if (!Object.prototype.hasOwnProperty.call(doc, item.name)) { | ||
console.error(`Wrong template config? Doc property ${item.name} not found.`); | ||
return undefined; |
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:
Можно и проще, как ниже делал)
return undefined; | |
return; |
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.
if (layers.includes(undefined)) return; | ||
|
||
return layers; | ||
} |
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.
question:
А для понимания еще раз - что все таки собой эти слои представляют?)
|
Co-authored-by: Ilya Azin <martis.azin@gmail.com>
Co-authored-by: Ilya Azin <martis.azin@gmail.com>
@Krakazybik пока надо лично мне еще минимум час-два убить, чтобы доревьювить по мелочам + подправить темплейт и стили
|
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.
Выглядет рабочеспособно)
Как вольем - приступим к некст этапу 🚀
|
||
const config = getConfig(templatesDir); | ||
if (!config) return; | ||
|
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.
Дополню для других, что плагинах докузавра так и принято описывать инициализацию плагинов, так что тут все конвенционально +-)
const { id, title } = doc; | ||
|
||
const hashFileName = sha1(id + locale); | ||
|
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(to-improve, non-blocking):
А мб можно вынести функцию вне основной инициализации, чтоб не разбухала?
Или она на контекст основной функции завязана?
@@ -0,0 +1,23 @@ | |||
const sharp = require("sharp"); | |||
|
|||
function createImagePipeline(file) { |
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(to-improve, 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.
Комменты в коде, или здесь? =)
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.
в коде
} | ||
|
||
return font.getSVG(text, options); | ||
} |
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.
В коде комментарием тоже бы описать, достаточно неочевидный момент)
"sha1": "^1.1.1", | ||
"sharp": "^0.29.3", | ||
"superstruct": "^0.15.3", | ||
"text-to-svg": "^3.1.5", |
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.
thoughts:
Эх, вынести бы как-нибудь, чтобы зависимости только во время постбилда пригождались, и не лезли в основной билд 🤔
Но out-of-scope, мб другие что подскажут
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.
@Krakazybik мб ты что тут подскажешь, ты ведь достаточно много плагинов докузавра наизучал за это время)
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.
либо монорепу (лул), либо "sharp" "superstruct" "text-to-svg": можно в dev кинуть =) и отучить линтер матюгаться по import/no-extraneous-dependencies для папки плагина =)
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.
Ну пока так оставим, мб @Postamentovich @GhostMayor что-то подскажут
(для контекста опять же - это docusaurus-plugin, который чисто во время сборки нужен; потом отдельно в npm опубликуем наверное)
replace template validation error to templates initialization.
fs.readFileSync(`${templatesDir}\\${templateName}\\template.json`, encode), | ||
), | ||
})); | ||
|
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.
Тогда мб лучше в сам код занести)
@feature-sliced/core @feature-sliced/contributors Гляньте пож) Уже 3-ю неделю влить не можем |
@all-contributors please add @Krakazybik for code, plugin, tool |
I've put up a pull request to add @Krakazybik! 🎉 |
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.
Все ок, я только обратил бы внимание на работу с файловой системой.
Путь до конфига и ассетов лучше собирать через path
И лучше не использовать синхронные методы чтения.
Но так как это выполняется в постбилде, возможно это не очень актуально.
CHANGELOG
#251
Добавлено: Прототип плагина динамической генерации og картиночек.
Как это работает?
Для манипуляций с изображениями используется sharp работающий через
libvips
. На этапе postBuild, когда у нас всё собрано, получаем инфу из doc плагина и на её основе генерируем изображение с необходимыми нам дополнительными слоями. Сами изображения и слои описываем в наших шаблонах. Если нам нужно применить конкретный шаблон для конкретного документа - используем правила.