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

Fixes #2215: Turn effect_list.txt into effect_list.md #2230

Merged
merged 3 commits into from
Oct 7, 2018
Merged

Fixes #2215: Turn effect_list.txt into effect_list.md #2230

merged 3 commits into from
Oct 7, 2018

Conversation

akshat157
Copy link
Contributor

@akshat157 akshat157 commented Oct 1, 2018

Pull Request Prelude

Changes Proposed

Affected Branches:

stable
Issues addressed:
Turn the effect list documentation into markdown

Tracker number: #2215

Known Issues and TODO List

With respect to line 2 of Description of the effect list,
Is the (01f3 <ID> ....... ) supposed to be (0x1f3 <ID> ....... ) ?
If not, then this file is ready to be reviewed.

	- New file: effect_list.md added with
	  markdown version of previous text document.
@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@Helianthella Helianthella changed the base branch from stable to master October 1, 2018 14:17
Copy link
Member

@Helianthella Helianthella left a comment

Choose a reason for hiding this comment

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

thanks for your contribution! 👍

@Helianthella Helianthella added type:enhancement Issue describes an enhancement or feature that should be implemented component:documentation Affecting the documentation in the doc/ folder labels Oct 1, 2018
@Helianthella Helianthella added this to the Release v2018.10.21 milestone Oct 1, 2018
@akshat157
Copy link
Contributor Author

You're welcome! 🙂

@Emistry
Copy link
Member

Emistry commented Oct 3, 2018

Could we add those effect constant name into the table as well?
Its odd to always refer two files just to get the right effect and the constant name since we shall encourage user to use constant name rather than effect id.

Number ID Constant Name Description
1 EF_NAME Description

@Helianthella
Copy link
Member

Helianthella commented Oct 3, 2018

@Emistry good idea, let's do it!

what could also be interesting would be to add an added in and a removed in column so we could know which clients can or can't display the effect... but this might be too much work

@Emistry
Copy link
Member

Emistry commented Oct 3, 2018

I updated it with the constant name column.
Not sure which client work/not working for each effects, so I didnt add the 2 columns you mentioned.

Effect ID from 968+ has no description, I am suck at giving description, maybe someone else could help.

966 | Bone crack
967 | Another little explosion
Effect ID | Constant Name | Description
--: | :------------------------------| :----------------------------------
Copy link
Member

@Helianthella Helianthella Oct 3, 2018

Choose a reason for hiding this comment

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

the table should be properly aligned:

  ID | Constant Name                  | Description
----:|:-------------------------------|:----------------------------------
  -1 | EF_NONE                        | (none)
   0 | EF_HIT1                        | Regular Hit
   1 | EF_HIT2                        | Bash

And if you want to use Effect ID, the whole first column will have to be enlarged

Copy link
Member

Choose a reason for hiding this comment

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

Updated.

- Updated effect list based on db/constant.conf
- Added Constant Name column
Copy link
Member

@MishimaHaruna MishimaHaruna left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I'll be merging this now

@MishimaHaruna MishimaHaruna merged commit 09eb7fe into HerculesWS:master Oct 7, 2018
@akshat157 akshat157 deleted the effect_list.md branch October 7, 2018 17:11
@Emistry Emistry mentioned this pull request Oct 9, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:documentation Affecting the documentation in the doc/ folder type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants