-
Notifications
You must be signed in to change notification settings - Fork 59
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
Convert Civs to Declarative Config #15
Conversation
js/civs.js
Outdated
} | ||
|
||
const civsConfig = { |
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.
@paulirish any thoughts on a test that would be not inconvenient to make sure this is a no-op? Like some snapshot we could take of the entire state?
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 would be ready to merge this, unless we want to work on this check before that.
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'll merge it now, the check can still be done afterwards if desired.
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.
Thank you for this pull request! Your changes should also make it easier to implement some kind of soft validation, like that every civ that has a tower upgrade disabled has the corresponding tower disabled as well. Cool stuff!
I have added a few review comments (mainly about formatting and ordering).
Concerning the cross-reference feature: A nice idea! It slightly conflicts with the overall concept that you have to select a civ in order to see its tech tree, but I think the benefit outweighs that.
Your branch seems to be missing the last three commits from master, where I added additional data information like Line of Sight etc. There is still textual information about attack and armor classes to be added, which means that together with the cross reference feature, the infobox will take up a lot more space than it did before, which is something to look out for.
This is very true. The box is getting bigger. @paulirish had the idea to put the cross reference in a That way it could be hidden be default. Personally I find the cross reference to be super useful, I navigate to other civs entirely via cross reference buttons instead of the dropdown, so having it behind a clickable drop down isn't useful for me, but I admit that the helptext is getting very large. But I can go into more detail in that PR, once this one is squared away! |
I am working on making a cross reference feature for the helptext.
In order to support that, I refactored the civilization config to be defined as a js object instead of calls to
enable()
,unique()
anddisable()
. This makes the config more declarative and easier to understand.The civ objects are laid out thusly:
In order to convert the config I made some helper scripts that you can see here and here.
You can preview the cross reference feature here: https://exterkamp.github.io/aoe2techtree/#Aztecs