Closed imjoshin closed 2 years ago
This pull request introduces 1 alert when merging 4a02980604fbe52fafc98ac681b261ab213aca3d into 634ed9827fce6cd3deb491cd86fc30947a6336a8 - view on LGTM.com
new alerts:
@dsrkafuu I did notice there's a bug in the PR as it stands where if you don't change the theme, this will break as theme
is now an object instead of a string. Do you know, if on upgrade, we reload settings or not? If not, I'll add a catch for the string theme
and re-parse it for backwards compatibility.
Nice theme. However, details of the theme do need a bit more adjustment, such as the hover color of the encounter time, etc. Since I've been busy recently, I'll start to make some changes when I'm free.
At the same time, I need to point out that for complex themes (e.g. not just changing the color), the size of the area occupied on the screen, the adaptation of the vertical mode, etc. may become a problem.
An important part is regarding the storage of theme-related settings: the ThemeOptions
part does not need to be put into the settings store, because it does not need to be exposed to the user (only the theme author), the settings will still only store the key, and using the key to get the relevant specific settings may be a good choice.
I likely won't revisit this, so if anyone's looking for this theme, feel free to reopen the PR!
This PR started as just a simple theme for making this overlay appear as a native dialog. Now, it extends the theming interface a bit to allow more options from individual themes. The goal of this change was to maintain the ease of use for color-changing themes, while also allowing for more complex theme customization.
The
ThemeOptions
type was added, which defines all available options. IndividualTheme
types within thethemes/index.ts
file are allow to set as many and as few options as the creator wants, using theThemePartial
type. Once the theme is ready to be rendered, we will now default all unset options using theDefaultThemeOptions
constants. These settings can be accessed using thesettings.theme
object from state, and all variables will be accessible on that object (set or not).I will note the Enix theme will need a bit more work to get it to look exactly native, but this is a good start.
Open:
Minimized: