MWPW-160237: Caas Bulk Publisher v2 (Beta) #3176
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
eslint
🚫 [eslint] <quotes> reported by reviewdog 🐶
Strings must use singlequote.
🚫 [eslint] <no-multiple-empty-lines> reported by reviewdog 🐶
More than 1 blank line not allowed.
milo/tools/send-to-caas/bulk-publish-to-caas.beta.js
Lines 492 to 493 in 47b08a5
|
Reminder to set the |
cmiqueo
added
trivial
|
This PR does not qualify for the zero-impact label as it touches code outside of the allowed areas. The label is auto applied, do not manually apply the label. |
There was a problem hiding this comment.
Looks great!
I'd prefer if all the root level code be moved out into it's own function(s). Even if just a giant setup function called first thing from init. Or if it needs to run before init, the setup function can be called from the root at bottom of the file near the init declaration.
Even better would be to break out each setup item into its own function so you get something like:
function setup() {
getPresets();
checkDarkTheme();
setupListeners();
etc...
}
| const successArr = []; | ||
| let index = 0; | ||
| let keepGoing = true; | ||
| const now = new Date(); |
There was a problem hiding this comment.
Can this just be inlined into getPageDom? I don't see any use case where the now param isn't new Date()
There was a problem hiding this comment.
@chrischrischris
Can you explain this more?
| if (!keepGoing) break; | ||
|
|
||
| try { | ||
| const rawUrl = page.Path || page.path || page.url || page.URL || page.Url || page; |
There was a problem hiding this comment.
nit might want to use case-insensitive lookup. Something like:
function getProp(obj, prop) {
const lowerCaseProp = prop.toLowerCase();
for (const key in obj) {
if (obj.hasOwnProperty(key) && key.toLowerCase() === lowerCaseProp) {
return obj[key];
}
}
return undefined;
}
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`ERROR: ${e.message}`); |
There was a problem hiding this comment.
Should this be pushed into the error array?
| const SIGNEDIN = BODY.querySelector('.status-signed-in'); | ||
| const SIGNEDOUT = BODY.querySelector('.status-signed-out'); |
There was a problem hiding this comment.
nit: add EL or some other indicator that it's an html element
| const SIGNEDIN = BODY.querySelector('.status-signed-in'); | |
| const SIGNEDOUT = BODY.querySelector('.status-signed-out'); | |
| const SIGNEDIN_EL = BODY.querySelector('.status-signed-in'); | |
| const SIGNEDOUT_EL = BODY.querySelector('.status-signed-out'); |
There was a problem hiding this comment.
Since this is already merged, I will be implementing these changes in my latest PR #3299
| if (caasEnvValue === 'prod' && !draftOnly.checked) { | ||
| publishWarning.style.height = '30px'; | ||
| } else { | ||
| publishWarning.style.height = '0'; |
There was a problem hiding this comment.
curious as to why not display: none
There was a problem hiding this comment.
Using height just for the CSS transition effect
| }; | ||
|
|
||
| // preset options | ||
| const presetsJsonPath = 'https://milo.adobe.com/drafts/caas/bppresets.json'; |
There was a problem hiding this comment.
This should probably not be in a drafts folder
There was a problem hiding this comment.
Good point. Will implement after beta
| const presetsJsonPath = 'https://milo.adobe.com/drafts/caas/bppresets.json'; | ||
| let presetsData = {}; | ||
|
|
||
| fetchExcelJson(presetsJsonPath).then((presets) => { |
There was a problem hiding this comment.
it's a bit strange that this just runs at the root level. Could you instead wrap it in a function and call from init?
| const useDarkTheme = localStorage.getItem('bp-theme') === 'dark' | ||
| ? true | ||
| : localStorage.getItem('bp-theme') === 'light' | ||
| ? false | ||
| : window.matchMedia('(prefers-color-scheme: dark)').matches; |
There was a problem hiding this comment.
Better to avoid nested ternary. Something like:
| const useDarkTheme = localStorage.getItem('bp-theme') === 'dark' | |
| ? true | |
| : localStorage.getItem('bp-theme') === 'light' | |
| ? false | |
| : window.matchMedia('(prefers-color-scheme: dark)').matches; | |
| const theme = localStorage.getItem('bp-theme'); | |
| const useDarkTheme = theme === 'dark' || | |
| (theme !== 'light' && window.matchMedia('(prefers-color-scheme: dark)').matches); |
| const useDarkTheme = localStorage.getItem('bp-theme') === 'dark' | ||
| ? true | ||
| : localStorage.getItem('bp-theme') === 'light' | ||
| ? false | ||
| : window.matchMedia('(prefers-color-scheme: dark)').matches; | ||
| /* eslint-enable no-nested-ternary */ | ||
|
|
||
| if (useDarkTheme) { | ||
| document.querySelector('.bulk-publisher').classList.add('dark'); | ||
| document.querySelector('#option-dark').checked = true; | ||
| } else { | ||
| document.querySelector('#option-light').checked = true; | ||
| } |
There was a problem hiding this comment.
nit break out into a function called from init
This is parallel beta version of the Caas BulkPublisher Tool
It provides a new and easier to use UI with predetermined Presets for the consumers.
Resolves: MWPW-160237 & MWPW-161613
Preset Mode:
Advanced Mode
Test URLs: