Skip to content

Conversation

SavePointSam
Copy link
Contributor

What:

Added an error message when there are no available roles.

Why:

Solves #328.

When unable to locate a role, an error message prints listing the available roles. When there are no available roles, the same message would print without a list of roles. This behavior is confusing/misleading to users.

How:

When the length of the string returned by prettyRoles is 0, we know there are no roles to be listed. The error message then changes based on that condition.

Checklist:

  • Documentation added to the
    docs site N/A
  • Typescript definitions updated N/A
  • Tests
  • Ready to be merged

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just one thing.

roleMessage = `
Here are the available roles:
${prettyRoles(container)
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid calculating the prettyRoles twice by putting roles in here instead of calling prettyRoles(container) again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I meant to do that. Haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected!

@kentcdodds kentcdodds merged commit d966008 into testing-library:master Jul 31, 2019
@SavePointSam SavePointSam deleted the pr/gh328_no-roles branch July 31, 2019 22:47
@kentcdodds
Copy link
Member

@all-contributors please add @SavePointSam for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @SavePointSam! 🎉

@kentcdodds
Copy link
Member

🎉 This PR is included in version 5.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants