Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

List treasure on Character Viewer #166

Merged
merged 5 commits into from
Sep 12, 2019
Merged

List treasure on Character Viewer #166

merged 5 commits into from
Sep 12, 2019

Conversation

marloso2
Copy link
Member

The Character Viewer shows the character's Money and Gems under a Treasure subheading in the Equipment tab

@datdamnzotz datdamnzotz added area/application Task related to orcpub application itself enhancement New feature or request labels Apr 27, 2019
Thank Zotz ;)
@datdamnzotz
Copy link

Also now I am not too sure about the sorting. Maybe we should leave it out, or recode it for ordering like the book does.
image

@datdamnzotz datdamnzotz changed the title Gold list on Character Viewer List treasure on Character Viewer Apr 28, 2019
Copy link

@KingMob KingMob left a comment

Choose a reason for hiding this comment

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

There's a mismatch between the component fn params and the render fn params. with-let is probably your best solution

@marloso2
Copy link
Member Author

marloso2 commented Jul 21, 2019

So that's the suggetsed improvements. Must say, I completely missed the .pointer. And thanks for the r/with-let, King. Either way, that leaves the question - Do we want alphabetical sorting, or no? I'm not sure yet how to do sorting to a custom order so someone else would have to do that once this is pulled

@datdamnzotz
Copy link

datdamnzotz commented Jul 21, 2019

Do we want alphabetical sorting, or no? I'm not sure yet how to do sorting to a custom order so someone else would have to do that once this is pulled

I would vote for if we can do it like the book sorts it.

@marloso2
Copy link
Member Author

I'm not sure yet how to do sorting to a custom order

I would vote for if we can do it like the book sorts it.

Welp, as I said, I have absolutely no idea how to do that so somebody else would have to 😋

@marloso2 marloso2 closed this Jul 23, 2019
@marloso2 marloso2 reopened this Jul 23, 2019
@codeGlaze
Copy link

Wait, isn't treasure already sorted in a logical order?

@marloso2
Copy link
Member Author

By default it's sorted in the order that it's added to your character. But we've also got the code for alphabetical sorting

@KingMob
Copy link

KingMob commented Jul 27, 2019

The treasure is stored in equipment.cljc in book order by default, but that doesn't help us sort a character's treasures. (Well, we could fake it by searching on strings and comparing indices, but that's slow and ugly.)

Instead, we should just make the order explicit, so it'll be available for sorting with sort-by.

E.g.:

(def treasure
  [{:key :cp
    :name "Copper Pieces (CP)"}
   {:key :sp
    :name "Silver Pieces (SP)"}
   {:key :ep
    :name "Electrum Pieces (EP)"}
   {:key :gp
    :name "Gold Pieces (GP)"}
   ...

becomes

(def treasure
  [{:key :cp
    :name "Copper Pieces (CP)"
    :rank 1}
   {:key :sp
    :name "Silver Pieces (SP)"
    :rank 2}
   {:key :ep
    :name "Electrum Pieces (EP)"
    :rank 3}
   {:key :gp
    :name "Gold Pieces (GP)"
    :rank 4}
   ...

and then if we have a sequence of character treasures with the :rank field in them, we can do:

(sort-by :rank some-character-treasure-vector)

I think that, or something similar, should work.

@marloso2
Copy link
Member Author

Ahh, that's quite a good solution, actually. I'm working on something else right now but next time I get fed up with that I'll come back to this PR and add the ordering 😋

@KingMob
Copy link

KingMob commented Jul 27, 2019

Another alternative is to place the character's treasure into a sorted map/set with sorted-map-by / sorted-set-by. Then, when you want to iterate over the map, they'll come out sorted.

@datdamnzotz
Copy link

@marloso2 see #231 as well.

@datdamnzotz datdamnzotz requested a review from KingMob September 12, 2019 18:54
@datdamnzotz
Copy link

I'm going to merge this to get the fixes into the branch. I don't think sorting should hold this up.

@datdamnzotz datdamnzotz merged commit da54299 into Orcpub:develop Sep 12, 2019
@KingMob
Copy link

KingMob commented Sep 23, 2019

Sorry @datdamnzotz, been real busy with life lately.

@codeGlaze
Copy link

@KingMob we've all been pretty quiet. I assumed that was the case all around

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/application Task related to orcpub application itself enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants