-
Notifications
You must be signed in to change notification settings - Fork 5
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
MWPW-147219 Generate content for validator #38
Conversation
bulk-update/bulk-update.js
Outdated
* @param {string[]} locales - An array of supported locales. | ||
* @returns {string} The staged URL. | ||
*/ | ||
export function localizedStageUrl(siteUrl, entry, stagePath, 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.
What if there isn't a stagePath
?
export function localizedStageUrl(siteUrl, entry, stagePath, locales = []) { | |
export function localizedStageUrl(siteUrl, entry, stagePath = '', locales = []) { |
Or could do an early 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.
Empty string works for now, if we come up with more use cases we can decide if we want an early return.
bulk-update/bulk-update.js
Outdated
fs.mkdirSync(outputDir, { recursive: true }); | ||
fs.writeFileSync(`${outputDir}/list.json`, JSON.stringify(config.list, null, 2)); | ||
|
||
const output = list.map((entry) => [`${siteUrl}${entry}`, localizedStageUrl(siteUrl, entry, stagePath, 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.
Better variable name might be stageOutput
or stageList
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.
Sounds good!
blog-test/migration.js
Outdated
[u('text', 'Entry'), u('text', entry)], | ||
[u('text', 'Date'), u('text', new Date().toISOString().split('T')[0])], | ||
]; | ||
const hideBlock = createBlock('Hide Block', fields); |
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.
It's a little confusing to name the block with a verb. As in, is it really hiding something or is it hidden? Might be more aptly name Hidden Block
if it's the latter. If this is just a test, it might not be important though.
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.
This block is display none in milo. I wanted to add a block like we did with card metadata but did not want it change anything visually.
https://github.com/adobecom/milo/blob/bmarshal/marketo-localized/libs/styles/styles.css#L723-L725
bulk-update/bulk-update.js
Outdated
const output = list.map((entry) => [`${siteUrl}${entry}`, localizedStageUrl(siteUrl, entry, stagePath, locales)]); | ||
fs.writeFileSync(`${outputDir}/staged.json`, JSON.stringify(output, null, 2)); |
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.
Should this be optional, only if stagePath exists?
const output = list.map((entry) => [`${siteUrl}${entry}`, localizedStageUrl(siteUrl, entry, stagePath, locales)]); | |
fs.writeFileSync(`${outputDir}/staged.json`, JSON.stringify(output, null, 2)); | |
if (stagePath) { | |
const output = list.map((entry) => [`${siteUrl}${entry}`, localizedStageUrl(siteUrl, entry, stagePath, locales)]); | |
fs.writeFileSync(`${outputDir}/staged.json`, JSON.stringify(output, null, 2)); | |
} |
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.
Thanks!
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.
Might add a README.md just to be in line with the other migration (blog-caas). Might also help clear up why you would want to add a hidden block to the page.
Resolves: MWPW-147219