KirkMcDonald / kirkmcdonald.github.io

Simple web-based calculator for the game Factorio.
Apache License 2.0
547 stars 146 forks source link

Added ability to navigate with menu buttons #61

Closed DarkMatterMatt closed 6 years ago

DarkMatterMatt commented 6 years ago

Added ability to navigate with browser forward/backward buttons as per #48

Also fixes furnace bug #60

DarkMatterMatt commented 6 years ago

Add load/save/reset settings #59 Settings page:

Save/load buttons are hidden when localStorage is no accessible

loadSettings now loads saved custom settings by default if all settings are default (if the user likes the default settings, then they can save default settings as custom settings so they aren't forced off the default settings or they can simply not save the custom settings)

Fixed bug where hash wasn't updated when browsing to calc.html (with no hash)

Moved hashchange detection initialization out of the DOMContentLoaded listener - it doesn't need to be there.

KirkMcDonald commented 6 years ago

FYI, I have just pushed 8330172f5f244d9248824a7f586b4c310f95aec8, which directly impacts portions of this PR and will require resolving the merge conflicts.

DarkMatterMatt commented 6 years ago

I don't like that this PR addresses at least three different issues. (Default settings, forward/back buttons, and the furnace setting thing.) This isn't enough to make me reject the PR, but in the future, please keep things more focused.

Sure, I (believe it or not) tried to. Do you know how to push future commits without them automatically being added here? It's my first time doing this pull request thing.

I'll work on your comments

KirkMcDonald commented 6 years ago

I am also not very familiar with how github does PRs, but I believe the PR applies to a specific topic branch in your fork (in this case, the master branch). So you could push changes to other branches, and they should not affect this PR.

DarkMatterMatt commented 6 years ago

Closing because I'm splitting browser navigation and setting saving into different pull requests (for personal experience as well as making the pull request less confusing)

DarkMatterMatt commented 6 years ago

I don't know how to PM you, so here's my question: should the load/save/reset settings also handle the mod/data-set or should it ignore it? I'm in two minds about it -

On one hand the mod is on the settings page, and everything else is being saved so it makes sense for the mod to be included (and the mod should be loaded by default if the user is using the mod).

On the other hand the mod is unlikely to be changed, and if the user changes it they probably don't want it to be changed back when they load their settings.

DarkMatterMatt commented 6 years ago

I'm also thinking that the ignore-list should be automatically stored in localStorage (in a future commit) because it is very unlikely that the user would want individual ignore-lists (and prefer to start with nothing ignored) for each design. The mod/data-set setting could be included with the ignore-list and be automatically processed separate from the other settings.

KirkMcDonald commented 6 years ago

I believe that the dataset must be part of the stored settings. In the future, when I add datasets other than vanilla, some of the settings will gain additional options when using those mods. For example, Bob's adds additional assemblers, and so the "minimum assembler" setting may be set to something that is not present in the vanilla dataset.

Indeed, the number of settings might itself change depending on the dataset, but that's something I'll be addressing in the future.

This is also why the settings tab re-renders itself whenever the dataset is changed.

As for the ignore list, I see it as being more akin to the user's selected items and modules than to their settings. The user is designing a factory, and I see the items they wish to ignore as being part of defining that factory, rather than a setting defining how they wish to use the calculator. If that makes sense. This distinction is somewhat vague, I admit, but I'm not convinced that this should be a saved setting.