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

Bazaar Helper #728

Merged
merged 14 commits into from
Jul 14, 2024
Merged

Bazaar Helper #728

merged 14 commits into from
Jul 14, 2024

Conversation

Emirlol
Copy link
Collaborator

@Emirlol Emirlol commented May 24, 2024

Highlights orders in the order menu. Gold color if there's coins, or dark green color if there's items to claim

image

I know the name sounds like it does a lot more than that, but it's just a humble highlighter, I just wanted it to sound cool. Maybe more stuff can be added later on, but I don't have any ideas for now.

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label May 24, 2024
@Julienraptor01
Copy link
Contributor

i would use a traffic light like color thing to say red expired, yellow partially completed and green completely completed personally, but nice already

@Emirlol
Copy link
Collaborator Author

Emirlol commented May 24, 2024

i would use a traffic light like color thing to say red expired, yellow partially completed and green completely completed personally, but nice already

I forgot order expiry is a thing, will add red later on. But I don't think partial completion or full completion highlight would help very much. If you claim the currently available items/coins, there's just nothing to for you to do or nothing that requires your attention to the highlighted item but it would still be highlighted.

@Julienraptor01
Copy link
Contributor

Julienraptor01 commented May 24, 2024

I forgot order expiry is a thing, will add red later on. But I don't think partial completion or full completion highlight would help very much. If you claim the currently available items/coins, there's just nothing to for you to do or nothing that requires your attention to the highlighted item but it would still be highlighted.

i think i wasn't clear when i meant partially completed, i mean items available but not fully completed
and fully completed mean items available and fully completed

@kevinthegreat1 kevinthegreat1 added the bleeding edge This PR has been accepted into bleeding edge label May 25, 2024
@Emirlol
Copy link
Collaborator Author

Emirlol commented May 26, 2024

image

@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label May 30, 2024
@Fluboxer
Copy link
Contributor

Fluboxer commented May 30, 2024

idea - different colors for finished order and partial order. Green for finished order, yellow otherwise

idea[2] - when buying/selling stuff show spread and highlight either instant or order button (like if spread is less than x% (default? lets be 5%) then you want to instasell/buy to not bother waiting, otherwise it may be worth making order and waiting)

upd: also green color on non-finished orders just to show type of order isn't good imo - hence why I suggest idea[2]
изображение

@Emirlol
Copy link
Collaborator Author

Emirlol commented May 30, 2024

idea - different colors for finished order and partial order. Green for finished order, yellow otherwise

Was planning on adding this, as a secondary highlighting scheme option.

idea[2] - when buying/selling stuff show spread and highlight either instant or order button (like if spread is less than x% (default? lets be 5%) then you want to instasell/buy to not bother waiting, otherwise it may be worth making order and waiting)

Great idea. I've always checked that manually and was bothered by it, yet the thought of automating it never came to me. Will give it a go.

kevinthegreat1
kevinthegreat1 previously approved these changes May 31, 2024
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels May 31, 2024
@kevinthegreat1
Copy link
Collaborator

Didn't test but looks good. I just realized you're still working on this so marking as wip.

@kevinthegreat1 kevinthegreat1 added wip This PR is a work in progress and removed merge me please Pull requests that are ready to merge labels May 31, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed wip This PR is a work in progress labels Jun 13, 2024
@BigloBot
Copy link
Contributor

This doesn't seem to work for Co-ops rn, possibly due to the bazaar screen being named differently depending on solo/co-op profile?

kevinthegreat1
kevinthegreat1 previously approved these changes Jun 27, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Still looks good :)

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jun 27, 2024
@kevinthegreat1
Copy link
Collaborator

Actually don't merge until coop is fixed lol.

@kevinthegreat1 kevinthegreat1 added changes requested This PR need changes and removed merge me please Pull requests that are ready to merge labels Jun 27, 2024
@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jul 9, 2024
@kevinthegreat1
Copy link
Collaborator

Looks good but there's some feedback about there being too much information. I suggested this.

@Emirlol
Copy link
Collaborator Author

Emirlol commented Jul 11, 2024

Looks good but there's some feedback about there being too much information. I suggested this.

That's not a bad idea, since you can't do anything to the expiry status without claiming stuff first.
I still think that knowing both at the same time is useful in rare cases such as long-running orders that you don't expect others to pass with their orders and that having both icons show up at the same time is a rare situation anyway, but it seems like most people don't share the opinion so I'll probably do what you suggested for the PR and make another branch for myself with the current one.

Also adds some convenience methods to SlotText to make the `SlotTextAdder` logic more readable
kevinthegreat1
kevinthegreat1 previously approved these changes Jul 13, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Some things if you want to address them.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 13, 2024
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed merge me please Pull requests that are ready to merge labels Jul 14, 2024
@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jul 14, 2024
@kevinthegreat1 kevinthegreat1 merged commit f34b2aa into SkyblockerMod:master Jul 14, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jul 14, 2024
@Emirlol Emirlol deleted the bazaar-highlight branch July 14, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bleeding edge This PR has been accepted into bleeding edge new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants