-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add latest and trending builds list to the build list module #7389
Add latest and trending builds list to the build list module #7389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission, this looks pretty interesting! I took a brief look at the code and the feature and I do like what I've seen so far. I'll start off by saying I'm usually pretty conservative about website tie-ins like this, for several reasons:
- Styling is easier/more freeform in a browser (e.g. "Open in PoB" button and you can style the list however you want)
- Maintenance in case the API changes
- Potential need to integrate with many build list provider websites and keep those up-to-date
- Security concerns about opening malformed builds or malicious links due to bugs/website neglect
Specific concerns for a build list website include the risk of baiting users into bad builds (especially non-beginner builds, given that lack of context available in a list like this), influencing the meta just based on how builds are sorted and (if multiple sites are implemented) which site is displayed by default, and build rot due to an outdated game version.
If we decide to integrate this, there are some changes that I'd like to see to improve it:
- Make the list website-agnostic. If we integrate pobarchives support, I can easily see others asking for support for their sites.
- Dropdown at the top showing the website this list is coming from
- More generic API
- Latest vs. Trending might not exist for every site
- Character name probably isn't needed
- Mapping file/function needed for each site
- Ascendancy assets can be found in TreeData. I'd love to be able to use those somehow instead of adding duplicate images.
- There's a scrollbar, but scrolling far enough stops showing builds.
- Styling of Trending vs. Latest is a bit misleading, as they're styled like tabs, but the active-looking tab is actually the inactive one (and is clickable)
- Some display issues when in portrait mode:
I haven't taken a fine-toothed comb to the code yet, but what I saw seemed pretty well done, so kudos for that and for a decent style otherwise!
* Make build list module responsive.
1ef4dd1
to
1cf2f59
Compare
@Wires77 Please take another look, requested changes are done, and I also implemented the similar builds list as I promised in the discord. New feature, Similar Builds
Fixes
Known issues, need helpPlease point me in the right direction for these issues, direct fixes by others are also appreciated.
Feel free to add new commits fellow PoB Warriors, I'm happy to share this PR and any help/improvement is appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small nitpicks on those buttons, but this already looks way better. The usual pattern in PoB is to create those buttons on class init and only draw them in the Draw function. Reason being Draw is called every frame so doing as little logic as possible in there is the better play.
- Content height was fine on similar builds but See All button in buildlist was partially hiding the import/preview buttons.
Co-authored-by: Wires77 <Wires77@users.noreply.github.com>
If anything, this should link directly to the build link that Divina scraped from, and not to PoB Archives. |
FYI This PR is a hot topic on reddit |
Description of the problem being solved:
Steps taken to verify a working solution:
Before screenshot:
After screenshot: