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

Replace toast with modal #358

Merged
merged 23 commits into from
Aug 10, 2024
Merged

Replace toast with modal #358

merged 23 commits into from
Aug 10, 2024

Conversation

sundaram123krishnan
Copy link
Collaborator

@sundaram123krishnan sundaram123krishnan commented Aug 8, 2024

Closes #353

@sundaram123krishnan sundaram123krishnan changed the title r Replace toast with modal Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 44.85294% with 75 lines in your changes missing coverage. Please review.

Project coverage is 28%. Comparing base (4989c48) to head (1f265a8).

Files Patch % Lines
src/modal_handler.rs 52% 58 Missing ⚠️
src/piggui.rs 0% 14 Missing ⚠️
src/views/hardware_menu.rs 0% 2 Missing ⚠️
src/views/version.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #358    +/-   ##
======================================
- Coverage      32%    28%    -4%     
======================================
  Files          37     37            
  Lines        4355   4266    -89     
======================================
- Hits         1393   1184   -209     
- Misses       2962   3082   +120     
Flag Coverage Δ
unittests 28% <45%> (-4%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewdavidmackenzie
Copy link
Owner

andrewdavidmackenzie commented Aug 8, 2024

When you make a change then try to exit, you get a dialog with a "Save" option.
Clicking that button does not Save, it just sends you back to the app.
So, I would either change the behaviour or rename it.
"Exit without saving" + "Return to app" (or similar working == cancel the exit)
i.e. I would loose the functionality where it remembers you tried to exit, and allows you on the second attempt.
Just have it give you the two options each time...

  • exit without save
  • return to app

Or, when you return to the app, pop-up the save dialog....

@andrewdavidmackenzie
Copy link
Owner

Unrelated to this PR but could fix at the same:

We should arguably align the button text "Show Hardware Details" and the dialog Title "About Connected Hardware"

Also, as the button pops up a dialog, I think it's text should have "..." at the end.

@andrewdavidmackenzie
Copy link
Owner

The version/details dialog looks good.

The contents of the hardware details looks good.

I don't think there are any other uses/cases, no?

@andrewdavidmackenzie
Copy link
Owner

Add the behaviour where ESCAPE key does close/cancel, like the connect dialog, and returns you to the app
(i.e. Escape should never dump you out of the app)

@andrewdavidmackenzie
Copy link
Owner

The text inside these two dialogs looks different somehow.
Same text size?
Same font?

Screenshot 2024-08-08 at 4 23 48 PM

Screenshot 2024-08-08 at 4 25 09 PM

@andrewdavidmackenzie
Copy link
Owner

It still has the "exit two times logic".

If I edit, try to exit, "return to app", (forget) then try to exit - then it exits directly.

Copy link
Owner

@andrewdavidmackenzie andrewdavidmackenzie left a comment

Choose a reason for hiding this comment

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

I have made chanegs I saw needed


#[derive(Clone, Debug)]
pub enum ModalMessage {
ShowModal,

Choose a reason for hiding this comment

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

Is this ever used?

Choose a reason for hiding this comment

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

I removed it

@sundaram123krishnan sundaram123krishnan merged commit c3eb5d5 into master Aug 10, 2024
13 checks passed
@sundaram123krishnan sundaram123krishnan deleted the pigg-353 branch August 10, 2024 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Toast with Modal
2 participants