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

Added option to hide toast messages #64

Open
wants to merge 14 commits into
base: test-adb-server
Choose a base branch
from

Conversation

aquirozc
Copy link

@aquirozc aquirozc commented Jan 7, 2023

A new option was added into the Other Settings section to enable users to hide toast alerts If they want to do so. For people running the app on wearables this could be really practical.

@aquirozc
Copy link
Author

aquirozc commented Jan 7, 2023

This commit was a follow up of this issue:

#35

@MailYouLater
Copy link

Looks to me like all the other boolean settings' variable names start with is, this is probably intentional so I'd recommend following the same naming convention. Perhaps something like isHideToastsEnabled, which would also fit better with the text description and the rest of your code.

@MailYouLater
Copy link

MailYouLater commented Jan 12, 2023

I will probably disable toast messages if/when I can, but I'm not sure that disabling them by default is a good idea. I think the toast messges are an important part of teaching new users how MATVT works... kind of like a tutorial, and while wearable users may be more likely to want to disable the toast notifications, I think they should still get that tutorial mode by default.

@aquirozc
Copy link
Author

aquirozc commented Jan 13, 2023

I think that renaming it to isHideToastsOptionEnabled should be the approach. It follows the previously established naming conventions and is grammatically correct.

@aquirozc
Copy link
Author

Sorry, the last commit wasn't meant to be pulled into the currently opened pull request. On most Desktop Operating Systems by pressing the middle click you'll switch into scroll mode, but with no pop-up being displayed, instead, the mouse icon will be replaced with some kind of "scroll icon". I'm planning to implement that feature into the MATVT project, however, isn't finished yet.

Reference image:
images

@MailYouLater
Copy link

That could be a cool enhancement, but as you said, it should probably be a seperate PR from this one.

@aquirozc
Copy link
Author

Yeah that is what I wanna do next, it was what I planed to do. However I new to Github so I through PR were like releases, once you submit a PR/Release further commits to source code wouldn't be bundled till next release.

@MailYouLater
Copy link

Ah, I see, you thought they were like tags. In fact, a Pull Request is an issue that has a branch associated with it. (There's even a way to attach a branch to an issue you've made to turn it into a PR.)
This allows people to iterate on and improve their contributions based on the feedback gained from code reviews in the PR and comments on its discussion page without having to close it and open a new PR. This is also one reason why creating a seperate branch to develop a seperate feature is often considered a good practice.

You might also want to learn about rebasing, though you should always be extra careful when doing any procedure that rewrites history, as it's deceptively easy to mess things up and lose data.

@virresh
Copy link
Owner

virresh commented Jan 17, 2023

Thanks for the contribution.
This one will need some cleanup before merging. I'm working on refactoring the test-adb-branch to make it work with the accessibility engine simultaneously. Will keep this pull request alive till then.

Feel free to add more commits here, I'll merge them together later.

@virresh virresh added this to the MATVT v1.0.7 Release milestone Jul 2, 2023
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.

3 participants