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

WebHost: Fix NamedRange dropdown being blank instead of Custom for presets #4063

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

remyjette
Copy link
Collaborator

What is this fixing or adding?

@agilbert1412 wrote up a very good description of the bug in https://discord.com/channels/731205301247803413/1295518127974776885.

This issue is caused because when applying presets, after we determine the true value of the NamedRange from the preset, we try to set the namedRangeSelect to that value. If the value doesn't exist, trying to do so unsets the select element. This change detects if it was flipped to unselected (.selectedIndex == -1) and changes it to "custom" instead.

How was this tested?

Ran the WebHost locally, loaded up the Stardew Valley player option page, applied the available presets.

@github-actions github-actions bot added affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Oct 16, 2024
@remyjette remyjette added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Oct 16, 2024
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

js is not my forte but the code change looks reasonable, also locally tested Stardew "Medium" preset and confirmed that the "Profit Margin" option changed to the correct custom value of 150

Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

For some reason, this almost-one-liner feels suboptimal to me. Like my instinct is telling me there is probably a cleaner way to do it.
But I've been trying to come up with one, and have been unsuccessful. So in the absence of an alternative, I think this is good enough!

@remyjette remyjette added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Oct 17, 2024
@Berserker66 Berserker66 merged commit ede59ef into ArchipelagoMW:main Oct 17, 2024
15 checks passed
@remyjette remyjette deleted the named-range-preset-blank branch October 17, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants