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

Zodiac Symbols #569

Merged
merged 6 commits into from
Feb 14, 2023
Merged

Zodiac Symbols #569

merged 6 commits into from
Feb 14, 2023

Conversation

seon22break
Copy link
Contributor

Hello, My name is Jhonatan and I would like to make this small contribution, which allows you to generate a random zodiac sign.
Reason:
Why not?

I like your project and for that reason I would like to contribute with a little bit of code.

@victorquinn
Copy link
Member

So I am supportive of this addition and thank you for your work here!

My only question is around the list of symbols you have to pick from. You have:

["Ram","Bull","Twins","Crab","Lion","Virgin","Balance","Scorpion","Archer","Goat","Water-Bearer","Fish"]

I am no expert on this stuff, but when I do a search, my first hit returns:

["Aries","Taurus","Gemini","Cancer","Leo","Virgo","Libra","Scorpio","Sagittarius","Capricorn","Aquarius","Pisces"]

And these are the ones I am more familiar with also. Was there a reason you had the list you did?

@seon22break
Copy link
Contributor Author

So I am supportive of this addition and thank you for your work here!

My only question is around the list of symbols you have to pick from. You have:

["Ram","Bull","Twins","Crab","Lion","Virgin","Balance","Scorpion","Archer","Goat","Water-Bearer","Fish"]

I am no expert on this stuff, but when I do a search, my first hit returns:

["Aries","Taurus","Gemini","Cancer","Leo","Virgo","Libra","Scorpio","Sagittarius","Capricorn","Aquarius","Pisces"]

And these are the ones I am more familiar with also. Was there a reason you had the list you did?

Hi! Thanks for your reply. I decide this nomenclature because i think that is more international (English) but is true that the second list is more recognisable. I change this!


```js
chance.zodiac();
=> 'Bull'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also update the docs to match the common Zodiac signs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, I forgot. Sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

chance.js Outdated Show resolved Hide resolved
Co-authored-by: Victor Quinn <mail@victorquinn.com>
@seon22break seon22break requested review from victorquinn and somejeff and removed request for somejeff and victorquinn February 12, 2023 12:50
Copy link
Member

@victorquinn victorquinn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the addition and the fix @seon22break !

@coveralls
Copy link

Coverage Status

Coverage: 95.567% (+0.009%) from 95.558% when pulling 89fb35a on seon22break:master into 9dde97f on chancejs:master.

@victorquinn victorquinn merged commit 8b8548f into chancejs:master Feb 14, 2023
victorquinn added a commit that referenced this pull request Feb 14, 2023
* New function to generate random zodyac symbols

* Add test to zodiac symbols function

* Generate documentation to zodiac symbols

* Rename Symbols with a common name

* Update documentation with the real signs

* Update chance.js

Co-authored-by: Victor Quinn <mail@victorquinn.com>

---------

Co-authored-by: Victor Quinn <mail@victorquinn.com>
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.

None yet

4 participants