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

Add warning modal dialog #1390

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

erri120
Copy link
Member

@erri120 erri120 commented May 21, 2024

Resolves #1376.

2024-05-21_12-48-54

@erri120 erri120 requested a review from a team May 21, 2024 10:59
@erri120 erri120 self-assigned this May 21, 2024
Copy link
Member

@Sewer56 Sewer56 left a comment

Choose a reason for hiding this comment

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

I think we may have a bug that affects this PR.
Either that, or we're missing something.

If I recall correctly, Community and Design (@Pickysaurus @captainsandypants) wanted for the dialog to show on every boot. And later down the road in 'Beta' we'd add a don't show again option.

That is actually the case right now, but I'm not sure if that's happening for the right reason.

In AlphaWarningViewModel.MaybeShow we try to obtain a previous instance of AlphaSettings from MnemonicDB, that is _settingsManager.Get<AlphaSettings>(). On my machine (with fresh DataModel), this seems to always return an AlphaSettings where HasShownModal is false. Even though in the previous run of the App we've set that to true.

Switching to the JSON backend seems to correct this behaviour.

public static ISettingsBuilder Configure(ISettingsBuilder settingsBuilder)
{
    return settingsBuilder
        .ConfigureStorageBackend<AlphaSettings>(builder => builder.UseJson());
}

In which case the dialog is not shown again (we get HasShownModal == true on subsequent boot).
Can you doublecheck that what I'm experiencing here is a bug with the MMDB settings backend?

@Sewer56
Copy link
Member

Sewer56 commented May 21, 2024

Oh and @captainsandypants ; currently the Show Changelog button will show the changelog under the current modal. If you have a design or desired behaviour for that stated anywhere, do let us know. Right now you won't really know it opened until you dismiss the modal.

<TextBlock Classes="BodyLGBold">Important</TextBlock>
</StackPanel>

<TextBlock Classes="BodyMDNormal" TextWrapping="Wrap">This app is still very early in development and currently only supports Stardew Valley. Please ensure no mods are installed in your game folder before testing.</TextBlock>
Copy link
Member

@Sewer56 Sewer56 May 21, 2024

Choose a reason for hiding this comment

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

Technically speaking these should be localized, given we will likely reuse this dialog for beta; which may have non-English people trying to participate, but it's not big enough a deal to block the PR. Am just commenting as it's worth pointing out.

@erri120
Copy link
Member Author

erri120 commented May 21, 2024

@Sewer56 can you explain your comment, I'm not sure I follow. The overlay will only appear once, similar to the telemetry dialog.

@erri120
Copy link
Member Author

erri120 commented May 21, 2024

And I just realized I forgot to hook up the help icon.

@Sewer56
Copy link
Member

Sewer56 commented May 21, 2024

@Sewer56 can you explain your comment, I'm not sure I follow. The overlay will only appear once, similar to the telemetry dialog.

The tl;dr is I reset my DataStore to get to a clean state and

  • Saving AlphaSettings with MnemonicDB backend did not work.
  • Saving AlphaSettings with JSON backend worked.

I was asking if this behaviour is only something that reproduces on my machine.

@erri120
Copy link
Member Author

erri120 commented May 21, 2024

@Sewer56 can you explain your comment, I'm not sure I follow. The overlay will only appear once, similar to the telemetry dialog.

The tl;dr is I reset my DataStore to get to a clean state and

* Saving AlphaSettings with MnemonicDB backend did not work.

* Saving AlphaSettings with JSON backend worked.

I was asking if this behaviour is only something that reproduces on my machine.

Sounds like a you problem 😅.

@Sewer56
Copy link
Member

Sewer56 commented May 21, 2024

Goddamn it 😅

@Sewer56
Copy link
Member

Sewer56 commented May 21, 2024

Ah, a full rebuild fixed it. Very strange.
In any case, do confirm with design if that's what they want.
I do think they wanted it to show on every boot till beta. Unless we've decided to go through with showing it once per version.

@erri120
Copy link
Member Author

erri120 commented May 21, 2024

Ah, a full rebuild fixed it. Very strange. In any case, do confirm with design if that's what they want. I do think they wanted it to show on every boot till beta. Unless we've decided to go through with showing it once per version.

From figma:

2024-05-21_14-40-56

@halgari halgari merged commit c2a7369 into Nexus-Mods:main May 21, 2024
7 checks passed
@erri120 erri120 deleted the feat/1376-first-start-dialog branch May 21, 2024 21:05
@erri120 erri120 mentioned this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants