Added ability to request gems by their value #1079
Conversation
|
could you update your fork? it appears to be out of date. For the future, it is better practice to create a branch on your fork before submitting a PR. That way you have the ability to work on other things while this PR is still under review. |
internal/caravan/tradeagreement.lua
Outdated
| TradeAgreementOverlay.ATTRS { | ||
| desc = 'Adds select all/none functionality when requesting trade agreement items.', | ||
| default_pos = { x = 45, y = -6 }, | ||
| default_enabled = true, | ||
| viewscreens = 'dwarfmode/Diplomacy/Requests', | ||
| frame = { w = 40, h = 4 }, | ||
| frame_style = gui.MEDIUM_FRAME, | ||
| frame_background = gui.CLEAR_PEN, | ||
| } |
There was a problem hiding this comment.
Inside of tables, the convention (at least inside of DFHack code) is to have no spaces around the =
internal/caravan/tradeagreement.lua
Outdated
| self:addviews { | ||
| widgets.HotkeyLabel { | ||
| frame = { t = 1, l = 0 }, | ||
| label = 'Select materials with value', |
There was a problem hiding this comment.
| label = 'Select materials with value', | |
| label = 'Select by value', |
internal/caravan/tradeagreement.lua
Outdated
| return matPrices, matValuesUnique | ||
| end | ||
|
|
||
| local function ValueSelector() |
There was a problem hiding this comment.
convention for local functions is snake_case, not PascalCase (which is generally used for class names)
internal/caravan/tradeagreement.lua
Outdated
| label = 'Select materials with value', | ||
| key = 'CUSTOM_CTRL_M', | ||
| on_activate = ValueSelector, | ||
| visible = function() return GetTab() ~= nil end, |
There was a problem hiding this comment.
since GetTab (or get_tab, once renamed) is a function, this can just be visible=get_tab
internal/caravan/tradeagreement.lua
Outdated
| list[index] = tostring(value.value) .. | ||
| " - " .. tostring(value.count) .. " " .. (value.count == 1 and currTab.labelSingular or currTab.labelPlural) |
There was a problem hiding this comment.
for multi-part strings like this, it is usually cleaner to use a format string:
('%d - %d %s'):format(value.value, value.count,
value.count == 1 and currTab.labelSingular or currTab.labelPlural)There was a problem hiding this comment.
I'd also suggest adding the "gear" symbol after the value to indicate price. the number-heavy string label also feels a bit user-unfriendly. we might want to post some more screenshots to the #feature-discuss Discord channel to get feedback on that.
internal/caravan/tradeagreement.lua
Outdated
| labelSingular = labelSingular, | ||
| labelPlural = labelPlural, | ||
| getCivResourceList = getCivResourceList, | ||
| funcGetValue = funcGetValue, |
There was a problem hiding this comment.
the convention for this type of variable is to name it like get_value_fn
|
ping |
No description provided.