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

Change how we ask questions #75

Merged
merged 11 commits into from
Jun 6, 2019
Merged

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Jun 4, 2019

Closes #52

Still WIP mainly because I haven't fixed the tests yet but thought I'd open this to get feedback The tests are fixed now.

changeset questions

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2019

🦋 Changeset is good to go

Latest commit: 2a7939c

We got this.

Not sure what this means? Click here to learn what changesets are.

@codecov-io
Copy link

codecov-io commented Jun 4, 2019

Codecov Report

Merging #75 into master will increase coverage by 0.43%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   82.75%   83.18%   +0.43%     
==========================================
  Files          31       31              
  Lines         800      809       +9     
  Branches      143      145       +2     
==========================================
+ Hits          662      673      +11     
+ Misses        130      125       -5     
- Partials        8       11       +3
Impacted Files Coverage Δ
packages/cli/src/utils/cli.ts 0% <0%> (-4.77%) ⬇️
packages/cli/src/commands/add/createChangeset.js 79% <68.18%> (-2.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f71188c...2a7939c. Read the comment docs.

@emmatown emmatown marked this pull request as ready for review June 4, 2019 22:25
Copy link
Collaborator

@Noviny Noviny left a comment

Choose a reason for hiding this comment

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

This looks Aces.

My two biggest questions are:

'Switching from inquirer to enquirer', - I'm going to do some testing based on being able to filter packages still for Very Long Lists.

Second was the slightly awkward 'we will patch these, yes or we re-ask this forever'. I think that:

Summary -> major -> minor -> confirm

solves that. Having thought out the summary text might also help selecting packages for the changeset. But I guess can also do the opposite so shrug

while (!shouldTheRestOfThePkgsBeMinorBumped) {
logger.error("The rest of the selected packages must be patch bumped");
shouldTheRestOfThePkgsBeMinorBumped = await cli.askConfirm(
bold(`Are you okay with these packages having a ${blue("patch")} bump?`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. I'm wondering if we want to not ask about patch here, as this can sort of be covered by 'is this your summary'

If we do that, we could move to summary -> all Packages -> major -> minor -> confirm - I'm not entirely sure the implications of that and might be something to come back to, not for this PR.

Otherwise you go major -> minor -> summary -> confirm, and while this is technically fine (without this question), it feels odd not to get asked about patches?

Maybe we just log it out the information:

The rest of the packages will be released as a patch:
#list

and move on to the summary question.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've gotten rid of the question and kept the logging of the packages.

{
name: "unchanged packages",
choices: unchangedPackagesNames
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯 This is both infinitely more readable as code, and also way more practical as a thing.

@@ -0,0 +1 @@
Improve how the CLI asks questions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we expand this a little more into how it's changed? I'd almost add the gif, but also the gif is 7MB and I don't really want to carry that around with this repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the text and embedded to the gif from the GH user content url so it's not in the repo and unless GH dies, it'll be accessible.

"@types/semver": "^6.0.0",
"@types/uuid": "^3.4.4",
"bolt": "^0.22.1",
"boxen": "^1.3.0",
"chalk": "^2.1.0",
"cli-table": "^0.3.1",
"detect-indent": "^6.0.0",
"enquirer": "^2.3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is why things are nicer. I've been wondering why all the changes.

@Noviny Noviny merged commit 16fde2e into master Jun 6, 2019
@emmatown emmatown deleted the mitchellhamilton/question-changes branch June 6, 2019 02:28
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.

Change how we ask questions
3 participants