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

Adds Trinket Compare #113

Draft
wants to merge 29 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Ellimistdev
Copy link

@Ellimistdev Ellimistdev commented Nov 21, 2023

Needed a good way to see a trinket's impact across multiple specs.

Adds:

  • Top level nav "Trinkets"
  • New Trinket submenu to swap between item, item level, and fight styles

trinkets_balefire_branch_454_castingpatchwerk
trinkets_caged_horror_486_castingpatchwerk3

Outstanding todos:

MVP:
- Hook into db instead of fetching everything like a goblin
- Need to add trinket-specific queries instead of needing to get ALL trinkets for ALL specs 😒
- Server-side data handling

Midterm:

  • Better trinket selection dropdown (scrolling at a minimum)
  • Item_level handling for non-existent item levels for items
  • Update fight_style in chart title.
  • Trinket menu colors? Just picked Rogue. is now priest

Longterm:

  • Swap item_name for item_id throughout to aid in localization
  • Drilldown links: trinket_compare to trinkets

@Bloodmallet
Copy link
Owner

This looks great! Let me add some information:

  • the db does not "know" about simulation results and their inner workings. it is aware of where their jsons exist. therefore fetching all simulation results "like a goblin" is the way to go
  • server side tries to be as uninvolved with game data as possible. it tries to let bloodytools in cooperation with simc_support do their thing and just pipe the result through to the frontend js

Midterm:

Better trinket selection dropdown (scrolling at a minimum): [this](https://getbootstrap.com/docs/4.0/components/dropdowns/#options)  should hopefully help
Item_level handling for non-existent item levels for items: when collecting trinket information each trinket should already know about which itemlevels are present for it. this can be used to filter the list or create it
Update fight_style in chart title.: 👍 
Trinket menu colors? Just picked Rogue.: I'd probably pick priest because it's the most neutral "color"

Longterm:

Swap item_name for item_id throughout to aid in localization: 👍 
Drilldown links: trinket_compare to trinkets: sounds great!

@Ellimistdev Ellimistdev marked this pull request as draft December 4, 2023 20:02
@Ellimistdev
Copy link
Author

Finally picked this back up!

I reworked this so that it does not constantly self-crawl the site. Given that we currently receive all item data for any one spec, to generate these charts, we only need to hit each spec once and cache the result in localstorage (updated every 30 minutes). Additionally, I extracted several functions from bloodmallet_chart_import() to reduce complexity and improve reusability.

As far as I can tell, these changes don't impact existing functionality, though I have not ventured into accounts and custom charts.

TrinketCompare

@Ellimistdev Ellimistdev marked this pull request as ready for review April 11, 2024 00:41
@Ellimistdev Ellimistdev requested a review from Bloodmallet April 11, 2024 17:17
@Bloodmallet
Copy link
Owner

Sounds great! I'll need some time to review it. I'm currently busy preparing my own chart implementations and that's occupying my attention. I'll hopefully get back to you soon on this. 👍

@Ellimistdev Ellimistdev marked this pull request as draft June 22, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants