LittleLightForDestiny / littlelight

Little Light is an inventory manager/companion app for Destiny 2 for both iOS and Android. It helps guardians move their gear and track their ingame progress.
MIT License
159 stars 32 forks source link

Added partial level progress bar, show power level diffs on items, fixed achievableAverage #219

Closed bill0042 closed 1 year ago

bill0042 commented 1 year ago

Added progress bar to show fractional amount over level. Added difference values to each max power item. Fixed issue in achievableAverage function. Changed it to use int math to avoid any rounding issues, continue looping only with whole level increases. Changed achievedPowerfulTier and achievedPinnacleTier to use >= instead of >

TheBrenny commented 1 year ago

Love your work and the idea of a progress bar! Pending that review, lgtm!

There are some changes to the design, though, and while I'm not the most creatively-minded person in the world, I'd be curious to see what they look like without having to clone and build. Any chance you could post a screenshot of what this looks like?

As an aside, please don't close and open new PRs when you make more changes - it just pings everyone who subs to those events. Instead, you can make changes directly to the branch which you're requesting the PR from, and the PR will update accordingly, including invalidating any outdated code reviews/comments.

bill0042 commented 1 year ago

Thank you for the feedback. Sorry about the pings. I was thinking that it would be easier to read the diffs if I remade the PR since it combined the changes together. So adding to the existing PR doesn't ping everyone? I sent you a screenshot through discord.

TheBrenny commented 1 year ago

Nah, adding to the existing PR only pings those subbed to the PR (like you and me now) as well as those subbed to ALL events (maybe Joao).

And Git and GitHub manage the diffs cleanly. If they think it doesn't diff well, then they chunk it into bigger diff blocks.

bill0042 commented 1 year ago

I have made and committed the suggested changes except for using the red color on per item diffs and the formatting of the diffs. I ended up using a custom red color to get the best readability. The stepProgressBar lib is gone and turned out to be easy to replace. Thank you for taking the time to show me how to do things better. There is a huge amount of very professionally done code in this app and I am very impressed by all the excellent work that went in to it.

joaopmarquesini commented 1 year ago

The thing about the colors is... if they are hard to read, then we should fix the theme itself to make it readable instead of using custom colors from MD Colors (even if that results as the theme providing colors directly imported from MD Colors). I've used custom colors from the beginning of the project (there's a lot of ocurrences there if you search for Color.) and they tend to become... very hard to track and keep standards over time. Not a merge blocking issue tho as that already happens all over the code, and maybe it depends on a proper redesign on the theme error colors.

bill0042 commented 1 year ago

I took a look at your color changes and it looks much better now! Very clever to use the background to show the red and green. The only issue now is that it doesn't show when max level is reached. I'm hesitant to change this because of how it will look the next time the power level increases (it won't next season) if you haven't updated the json in time. I think it might be a good idea to associate the power caps with the season number in the json so the new caps can be put in place ahead of time and take effect automatically when a new season is detected by finding the highest season number in DestinySeasonDefinition with a start date before the current date.

joaopmarquesini commented 1 year ago

tbh I'm not sure if that is really an issue, as the widget will be broken/show wrong results either way until I update the season caps (which is what happens right now anyway). I think that showing when max level is reached is an upgrade from what we have now even if it ends breaking up something that is already "broken".