Update Interface - Information.lua#2277
Conversation
Update for ATTWoWAddon#2275 -- look, I get that you guys will have a better way to do this, but I just mostly wanted to do a bit of UX cleanup and figure out how things worked inside of ATT. I had a bit of trouble because it's not consistently saving the faction so I did a bit of a hack based on race. It's fine, it workes for TBC and Classic. Cheers!
|
Hey @DFortun81, I made the changes you suggested. Thanks, works like a charm. 
I still think it'd be great to have a "Useful for" section showing you characters that could learn the recipe but haven't yet... but that can come later. |
|
Bit more realistic list. I went and purged all the old data I had from Era / HC / SoD. Still looks good. (= 
|
|
Hey @DFortun81 , Useful for added... but I gotta figure out how you guys are doing all the localization. 
I think it's useful; tells you if you need to buy a pattern when you are on an alt. (= |
|
Ok, so I added the "Useful for" section, but I just hard coded the Locale... if you guys like this I'll make the real change. Did a bit of a style tweak to make it more readable as well. Debated only showing for the faction that you're currently on... but anyone who plays both sides probably wants to compare. =P 
|
Cosmetic change.
ImUnicke
left a comment
ImUnicke
left a comment
There was a problem hiding this comment.
Various stuff can be removed as we already have existing data lookups for those relationships.
Also, I don't really like lots of the comments being removed and re-formatting of existing code into one-line statements. Maybe check the compare in the PR and put back those removed empty lines and explanation comments for future reference.
| -- Known By / Completed By / Useful For | ||
| local KnownByIgnoredTypes = { | ||
| Achievement = app.IsRetail, | ||
| BattlePet = true, | ||
| BattlePetWithItem = true, | ||
| Illusion = true, | ||
| IllusionWithItem = true, | ||
| Mount = true, | ||
| MountWithItem = true, | ||
| } |
There was a problem hiding this comment.
Looks like this table got copy-pasted and is now defined twice
| local SKILL_TO_SPELLID = { | ||
| [171] = 2259, [164] = 2018, [185] = 2550, [333] = 7411, [202] = 4036, | ||
| [129] = 3273, [356] = 7620, [182] = 2366, [773] = 45357, [755] = 25229, | ||
| [165] = 2108, [186] = 2575, [393] = 8613, [197] = 3908, | ||
| } |
There was a problem hiding this comment.
Think these mappings are already generated into ReferenceDB by the Parser, so would prefer to use them from there instead of defining a local table.
Try using app.SkillDB.SkillToSpell instead of this local table
| local FACTION_ID_TO_NAME = { | ||
| [1] = "Horde", | ||
| [2] = "Alliance", | ||
| } |
There was a problem hiding this comment.
Don't like the non-localized strings here, should assign the proper Global values from Blizzard
FACTION_HORDE or FACTION_ALLIANCE
| CreateInformationType("CompletedBy", { text = L.COMPLETED_BY:format(""), priority = 11000, HideCheckBox = true, Process = ProcessForCompletedBy }); | ||
| CreateInformationType("KnownBy", { text = L.KNOWN_BY:format(""), priority = 11000, HideCheckBox = true, Process = ProcessForKnownBy }); |
There was a problem hiding this comment.
Think these got duplicated... I seem them down on lines 1243/1244 as well
|
Also I think we would need a Settings toggle for this sort of change, or as a mutually-exclusive Information Type. It's nice to see characters divided up this way, but I think in some situations it would easily bloat the tooltip so much that users would just turn off the info altogether. |
| local itemID = reference.itemID | ||
|
|
||
| if itemID and not spellID then | ||
| local _, _, _, _, _, _, spellIdFromItem = GetItemSpell(itemID) |
There was a problem hiding this comment.
These global WoW API calls (GetItemSpell/GetItemInfo) will probably not work in Retail or will be deprecated and removed in a future patch since most APIs are being namespaced. So we would need the proper APIs hooked into app.WOWAPI (see the WoW API Wrappers file) for proper game versions, and then use those here.
But really, seems you have all this spell checking and item lookup but you're just wanting the skillID or requireSkill value anyway since it should always be on an actual Recipe in ATT. I'd rather that be the only needed check to look up the associated profession spell to reference character data about their current skill level.
|
Hey, first of all, thanks for taking the time to review this. I just hacked something together that works on my local setup. That said, you clearly support more versions of the game than I do. Because of that, I want to be upfront. I do not care about Retail, and if retail support is a hard requirement, I am not the right person to fix this. When I was digging through the tooltip file, I noticed you have a pretty large team working on All The Things. If someone else is interested in this idea, I would genuinely love to see them run with it. What I was trying to address comes down to two pain points. First, the current “blob” of character names. Since recipes cannot be mailed to characters on other servers or factions, a single mixed list is not very useful. Splitting it by Horde versus Alliance and by server makes the information actionable. 
Second, I wanted a fast way to see which character a recipe is still useful for. When you encounter a pattern in the wild, you should not have to guess whether it is worth buying. The mixed faction list makes this harder than it needs to be. 
I fully admit these issues are most noticeable if you play both factions (which I understand is not a huge slice of the population), or play Era / SoD / Hardcore (which again I understand is not a huge slice of the population). Solving these pain points makes it easier to collect things. (= I personally use Recipe Master to solve the second point (although it has not been updated for The Burning Crusade yet). So all that said, I love the add-on! Thanks again for the work you all put into it. Cheers! |
2 participants
Update for #2275 -- look, I get that you guys will have a better way to do this, but I just mostly wanted to do a bit of UX cleanup and figure out how things worked inside of ATT.
I had a bit of trouble because it's not consistently saving the faction so I did a bit of a hack based on race. It's fine, it workes for TBC and Classic. Cheers!
Revised
Old