-
Notifications
You must be signed in to change notification settings - Fork 13
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
Refine prompts and output #71
Conversation
|
||
// Join Discord and GitHub | ||
Logger.info('💬 Join our Discord community to find answers to your issues or queries. Or just join and say hi.'); | ||
Logger.info(colors.cyan(' https://discord.gg/SN8Da2X'), '\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.
Skipping GitHub is intentional here? Since the comment above still mentions GitHub.
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.
Yes, displaying the Github link adds no real value to the output.
"eslint": "^8.17.0", | ||
"chromedriver": "^107.0.3", | ||
"eslint": "^8.27.0", | ||
"mocha": "^10.1.0", |
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 think chromedriver
and mocha
are not required in this project as a dependency.
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'm using mocha to run the tests, in addition to nightwatch.
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.
At the moment there is only one test failing if you run it with mocha and I think that having mocha as an option to run the unit tests locally it's useful.
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.
updated browsers prompt.
src/constants.ts
Outdated
|
||
// BROWSERS | ||
{ | ||
type: 'checkbox', | ||
name: 'browsers', | ||
message: 'Which browsers will you be testing on?', | ||
message: 'Select target browsers', | ||
choices: (answers) => { | ||
let browsers = BROWSER_CHOICES; | ||
if (answers.backend === 'local' && process.platform !== 'darwin') { |
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 (answers.backend === 'local' && process.platform !== 'darwin') { | |
if (process.platform !== 'darwin') { |
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.
But this will also remove 'Safari' for users who want to test on Safari browser on remote/cloud service from Windows/Linux machine.
Maybe we can just remove this if
block so that 'Safari' appears everytime, or if the user's system is non-mac, we can change the option to something like 'Safari (cloud only)'?
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.
Adding Safari (cloud only) will only complicate things further because we have to handle the case where you select Safari and do not select cloud. So I think for now it's ok to just show Safari for Mac because advanced users who want to test on cloud safari can always add the config manually.
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.
The case of selecting Safari and then not selecting cloud is already handled though, in refineAnswers
method.
(refineAnswers
is written in a way that even if there are discrepancies in the input answers, the output should always be in the right format.)
But I am okay with both approaches.
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.
so an error will already be thrown?
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.
No, 'safari' is silently removed from local browsers list if the user is on a non-mac machine.
Co-authored-by: Priyansh Garg <39924567+garg3133@users.noreply.github.com>
f595ed0
to
a992c9f
Compare
@vishalshah133 any thoughts here? this would be needed to be merged as @harshit-bs is already working on adding support for component testing. |
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.
LGTM! Just waiting on @vishalshah133 to review the questions flow once.
Renamed most of the prompts to match a more active voice and tone in order to simplify the onboarding process overall. This is also according to our own documentation style guide, namely:
Changes reflect a more imperative style:
New onboarding process:
Other changes:
The only prompt which I am not very sure of is "Select where to run Nightwatch tests"; to users who aren't familiar with Selenium or cloud testing services this prompt will most likely be confusing. We should probably change it to be similar to the one for mobile testing setup, e.g.: