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.

Sign up for GitHub

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

Jump to bottom

WebHost: Make list options available from the player-options page #2567

Closed
wants to merge 2 commits into from
Closed

WebHost: Make list options available from the player-options page #2567

wants to merge 2 commits into from

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Dec 5, 2023 •
edited
Loading

With this, players no longer need to go to the weighted options page
to set (or even discover) the Item Pool, Item & Location Hints, and
Priority & Exclusion Locations options, or any list-based options
defined for an individual game.

This also unifies more of the rendering code for player and weighted
options, so it'll be easier to share UI for new option types between
them in the future.

In a follow-up PR, I intend to make it possible for individual options
to specify whether they're visible on both options pages, the
weighted-options page only, or only usable from YAML.

How was this tested?

I manually tested loading up several pages, setting options,
refreshing to ensure that the options persisted in local storage, and
exporting the options as YAML.

If this makes graphical changes, please attach screenshots.

image

@ScootyPuffJr1 ScootyPuffJr1 added the affects: webhost Issues/PRs that touch webhost and may need additional validation. label Dec 5, 2023
@ThePhar ThePhar changed the title [WebHost] Make list options available from the player-options page WebHost: Make list options available from the player-options page Dec 5, 2023
@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label Dec 5, 2023
@PoryGone
Copy link
Collaborator

PoryGone commented Dec 5, 2023

Maybe I'm in the minority here, but I think that for most games, this page is already overwhelming to new users, and that this PR makes that significantly worse.
I'm of the opinion that start_inventory, priority_locations, and excluded_locations are more advanced settings, that don't need to be immediately exposed in this way.
It would be nice to see efforts made to make this page more approachable and useable before we start cramming more junk into it.

@nex3 nex3 force-pushed the more-player-options branch from ba9a701 to 40ade0b Compare December 5, 2023 22:06
@Alchav
Copy link
Contributor

Alchav commented Dec 6, 2023

Maybe I'm in the minority here, but I think that for most games, this page is already overwhelming to new users, and that this PR makes that significantly worse. I'm of the opinion that start_inventory, priority_locations, and excluded_locations are more advanced settings, that don't need to be immediately exposed in this way. It would be nice to see efforts made to make this page more approachable and useable before we start cramming more junk into it.

It is very underwhelming for experienced users. I would very much like to have the ability to create yamls on the website with all settings and not have to use the weighted settings page, the sliders for which I find very annoying, especially when I'm away from my computer and need to make a yaml on mobile

@PoryGone
Copy link
Collaborator

PoryGone commented Dec 6, 2023

Maybe I'm in the minority here, but I think that for most games, this page is already overwhelming to new users, and that this PR makes that significantly worse. I'm of the opinion that start_inventory, priority_locations, and excluded_locations are more advanced settings, that don't need to be immediately exposed in this way. It would be nice to see efforts made to make this page more approachable and useable before we start cramming more junk into it.

It is very underwhelming for experienced users. I would very much like to have the ability to create yamls on the website with all settings and not have to use the weighted settings page, the sliders for which I find very annoying, especially when I'm away from my computer and need to make a yaml on mobile

I get where you're coming from, but power users are significantly less likely to be fully turned off of using the platform when their needs aren't met than new users are, and as such, the user experience of the new user should take precedence when both cannot be simultaneously met.

Regardless, this has already been discussed at length in the Discord server, and Farrak has voice there his own concerns with this PR.

@ReverM
Copy link
Contributor

ReverM commented Dec 24, 2023

Could it be possible to have the player options page have a tab for basic and advanced?

remyjette
remyjette reviewed Jan 10, 2024
View reviewed changes
WebHostLib/static/assets/player-options.js
Comment on lines +91 to +92
const leftOptions = Object.fromEntries(entries.slice(0, Math.floor(entries.length / 2)));
const rightOptions = Object.fromEntries(entries.slice(Math.floor(entries.length / 2) + 1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will cause the middle option to be omitted from the page

WebHostLib/static/assets/player-options.js
randomButton.classList.add('randomize-button');
randomButton.setAttribute('data-key', option);
randomButton.setAttribute('data-tooltip', 'Toggle randomization for this option!');
randomButton.addEventListener('click', (event) => toggleRandomize(event, range));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and all the later calls to toggleRandomize should be this.#toggleRandomize

@PoryGone
Copy link
Collaborator

If I'm not mistaken, discussion of this took place in the Discord server, and forthcoming work from @LegendaryLinux will make this PR obsolete.

@PoryGone PoryGone added the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Feb 10, 2024
@LegendaryLinux
Copy link
Member

PoryGone is correct. I have an in-progress PR which all but removes JS from player-options and drastically cuts down on the JS in weighted-options.

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: other Issue/PR is waiting for something else, like another PR. labels May 2, 2024
@ThePhar
Copy link
Member

ThePhar commented Jun 1, 2024

I'm going to close this PR since it's been obsoleted by #2614.

@ThePhar ThePhar closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remyjette remyjette remyjette left review comments

Assignees
No one assigned
Labels
affects: webhost Issues/PRs that touch webhost and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. waiting-on: other Issue/PR is waiting for something else, like another PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nex3 @PoryGone @Alchav @ReverM @LegendaryLinux @ThePhar @remyjette @Exempt-Medic @ScootyPuffJr1