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

Add preview, build, and playbook scripts #10

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

Conversation

MikelD333
Copy link

@MikelD333 MikelD333 commented Oct 26, 2022

Fixed #4

@@ -40,5 +52,5 @@ function faviconsTags(htmlWebpackPlugin) {
.filter((tag) => tag.tagName !== 'script')
.join('')
// It must be Handlebars template with root path as variable
.replaceAll('uiRootPath', '{{{uiRootPath}}}')
.replace(/uiRootPath/g, '{{{uiRootPath}}}')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need a regexp?

@@ -5,10 +5,19 @@ const ZipPlugin = require('zip-webpack-plugin');
const FaviconsPlugin = require('favicons-webpack-plugin')
const CopyPlugin = require('copy-webpack-plugin')
const HtmlPlugin = require('html-webpack-plugin')
const { CleanWebpackPlugin } = require('clean-webpack-plugin')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't using a style that uses a few lines above? 🤔
I think CleanPlugin is more good name.

@@ -5,10 +5,19 @@ const ZipPlugin = require('zip-webpack-plugin');
const FaviconsPlugin = require('favicons-webpack-plugin')
const CopyPlugin = require('copy-webpack-plugin')
const HtmlPlugin = require('html-webpack-plugin')
const { CleanWebpackPlugin } = require('clean-webpack-plugin')
const path = require('path');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we merge this line with line 3 that already has a require('path')?

@@ -1,4846 +1,8 @@
{
"name": "taymyr-antora-ui",
"version": "1.0.0",
"lockfileVersion": 2,
"lockfileVersion": 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we downgrade a lockfile version? 🤔

"scripts": {
"preview": "npm-run-all -s build playbook",
"playbook": "antora antora-playbook.yml",
"build": "webpack"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that bundle is more semantic name and known to those who using Antora Default UI.

Suggested change
"build": "webpack"
"bundle": "webpack"

@@ -7,16 +7,24 @@
"type": "git",
"url": "https://github.com/taymyr/taymyr-antora-ui.git"
},
"scripts": {
"preview": "npm-run-all -s build playbook",
"playbook": "antora antora-playbook.yml",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think antora or site are more suitable. playbook doesn't mean anything 🤷‍♂️

@ihostage ihostage self-requested a review October 26, 2022 11:22
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.

Support preview command
2 participants