-
Notifications
You must be signed in to change notification settings - Fork 364
Prompt user if uploading settings_data.json #439
Conversation
e6f8a22
to
aa98c62
Compare
Pull |
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.
Tested on my side, everything checks out 🎩. Have some comments on the wording of things however.
packages/slate-sync/index.js
Outdated
return maybeDeploy; | ||
} | ||
|
||
function promiseThemekitConfig(files) { |
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.
Unused argument.
const figures = require('figures'); | ||
const flatten = require('array-flatten'); | ||
const minimatch = require('minimatch'); | ||
const wrap = require('word-wrap'); |
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.
Dependency not used.
)} It looks like you are about to upload the ${chalk.bold( | ||
'settings_data.json', | ||
)} file.\n` + | ||
` This can reset any theme setting customizations you've done in the\n` + |
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.
I say we remove the contractions and go with "you have" (we're using "you are" above).
` Or to disable this prompt, add the following to your slate.config.js file:\n`, | ||
); | ||
console.log( | ||
` ${chalk.cyan(`'slate-sync': { promptSkipSettings: false }`)}\n`, |
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.
Maybe it's me but I find the property name, and the description on line 46, slightly confusing.
First instinct by reading this: it should be promptSkipSettings: true
in order to disable the prompt. I understand what you're saying but I think it's the word "skip" that's throwing me off.
We should go with either promptSettings: false
or keep as is but rename to skipPromptSettings
.
id: 'promptSkipSettings', | ||
description: | ||
'Enable/disable the prompt to skip uploading settings_data.json', | ||
default: true, |
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 will need to be updated based on my comment above.
b5f505d
to
2dd31aa
Compare
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
What are you trying to accomplish with this PR?
Fixes #404
Still TODO
settings_data.json
in the.env
file.slate.config.js