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

Possible update #79

Open
theredsox opened this issue Sep 11, 2021 · 4 comments
Open

Possible update #79

theredsox opened this issue Sep 11, 2021 · 4 comments

Comments

@theredsox
Copy link

theredsox commented Sep 11, 2021

I forked this project last week because I'm interested in using the site locally for grail tracking in D2R. My main goal was to add the ability to track runes as part of the main grail progress. At the bottom you can see the change log entry for the work.

Don't know if any of the changes would be worthwhile for your main branch. As a React noob, there were a number of "hacks" that are probably bad form (like the global stylesheet). Either way figured I'd give visibility here. If interested in any part, go for it. If anyone is willing to review the changes and provide feedback in a 'new issue' on my branch, I'm open to feedback to learn better approaches and to clean things up.

Feel free to close out this issue once you've read it.

github.com/theredsox/d2-holy-grail/

"2.1.1": [
    {
      change: `Added rune tracking for grail, D2R images, and UI tweaks:`,
      children: [
        "Updated and added D2R images for all existing uniques, sets, runes",
        "Reorg of right side buttons and main menu",
        "Added rune tracking towards main grail",
        "Added flex layout to better utilize space at various screen sizes",
        "Reduced padding in various places to better utilize screen space",
        "Sets tab now uses a 'cards' layout, added completed set pics, and fullsize on click"
      ]
    }
  ],
@Nasicus
Copy link
Owner

Nasicus commented Sep 15, 2021

Hey thanks for taking the time to write this.

I had a quick look at the code, looks quite good.

The images are quite cool - are we allowed to use them?

Also could you show me some screenshots for some of the padding / spacing improvmeents you did? Yes I'm too lazy to clone your repo atm :D

If we really want to merge some from your stuff to here, I would appreciate it to have PR's only for one thing at a time.

The one thing from your changelog which I won't merge for sure is this one:

"Added rune tracking towards main grail",
Since the original grail also never included the runes directly.

But as for the other things - I am interested. Some pics for each feature would be nice, else I will probably have a look at it myself too once I have more time (and motivation :D) !

@theredsox
Copy link
Author

Wasn't sure you'd want the images anyway because they'll be much higher bandwidth used for the public site. But also have to admit I didn't mine them from the game data myself. I found them across 2 different websites, mostly purediablo.com and some through rankedboost.com. Both sites had nearly the same item images. Which leads me to assume they were mined from the game data. Unsure how that plays out ownership wise. If you want to merge for the public site, probably we'd want to contact those two sites to get permission. I might wait a bit though, many of the higher end items didn't seem to have unique images. As if maybe all the data wasn't available yet. So maybe a better set will be available after release. Maybe even something clearly labeled as free/shared use by Blizzard.

I finally finished my last big feature tweak, which I flagged as 2.1.3. Now when I log in as a party leader, I get an extra checkbox on the right top side called "show party view". When checked, it combines all party members data into a single grail to display in a read only mode. Allowing you to visually look around through the items and see what the party has and hasn't found. There's a couple hacks involved with that because I couldn't figure out the routing in a smooth way.

It calls a private method I made public because I couldn't figure out how to trigger it naturally through state change. The feature also jacks into the standard getGrail() call and passes an extra request flag which tells the server to serve up a party merged grail collection and flagged as read-only. You might like to see what I did though and implement it in a better way, maybe even in the party login instead. I did it in the standard login because it had all the layouts I wanted to use and figured I could hack the combined data in behind the scenes without having to figure out how the caching and state client side works too much. My server code there for combining the grails is probably slick enough to use if you decide to flush something out.

Here are some screenshots with various changes pointed out.

holygrail1
holygrail2
holygrail3
holygrail4
holygrail5
holygrail6
holygrail7
holygrail8

@Nasicus
Copy link
Owner

Nasicus commented Sep 23, 2021

Thanks.

I also played around little locally, but I must confess, that I found the item view without the cards simpler and cleaner.

I think the "party leader" view is quite cool, but as you said / your comment say it's a little bit hacky (not to say, that the existing code isn't, especially the state handling is quite ugly in this whole app ;D) and as mentioned I don't have a lot of time atm. Maybe I will look deeper it in the coming weeks.

I don't know what others think about the changes though....

Concerning the runes, I think there was a big discussion "once" (when I created this) and people didn't want runes in there, but if it would be a setting we could think about it ;D

Anyway cool work which you have done!

@theredsox
Copy link
Author

Thanks for poking around. No worries if you don't want to pull any of it over. Just felt obliged to offer. I appreciate the framework you put together. Worked well today tracking my first D2R drops :).

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

No branches or pull requests

2 participants