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 a view menu with zoom actions #16

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

crumblingstatue
Copy link
Contributor

Add the following actions:

  • Zoom in/out via menu (discoverability)
  • Reset zoom to default (800% currently)
  • Set zoom to 100%

Add the following actions:
- Zoom in/out via menu (discoverability)
- Reset zoom to default (800% currently)
- Set zoom to 100%
@crumblingstatue
Copy link
Contributor Author

More ideas:

  • Show shortcut keys for the menu items (see Button::shortcut_text)
    This would require making the shortcut data available to the menu function
  • For the reset zoom, inform the user of the value we are resetting to via hover
    This would require making the default zoom data available to the menu function

fn default() -> Self {
Self { default_zoom: 8. }
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I do like the idea of a preferences object, and we will need it, but for now I think it would be better to use just a const on the top of the file for this config. The reason I say this is that there is a lot of configuration scattered across the program, so when we do decide to implement a preferences or settings type we need to consider also those other values. There are lots of other considerations that I might open a discussion thread or an issue for this. With the constants all on top of the files it will be relatively easy to collect them once we decide how to implement the preferences system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll change it to a const when I get home.
It actually started out as a const, but I thought I would follow up this with a PR that makes the default zoom configurable, but maybe that shouldn't have leaked into this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, it's done. Let me know if you need any other change.

@yds12
Copy link
Owner

yds12 commented Mar 20, 2023

Thanks for taking the time to implement this very useful feature.

I like the proposals to show the shortcuts in the menu items as well, I'd welcome other PRs doing this. The hover with extra information about the default zoom is also great. Please see my comment in the PR code -- with that approach we also don't need to pass any information (for now, this will change later) to the menu, we can just access the constant.

@yds12
Copy link
Owner

yds12 commented Mar 20, 2023

Linking the preferences discussion thread: #17

@yds12
Copy link
Owner

yds12 commented Mar 21, 2023

Thank you! Merging

@yds12 yds12 merged commit db5c483 into yds12:master Mar 21, 2023
@crumblingstatue crumblingstatue deleted the zoom-menu-actions branch March 21, 2023 06:38
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.

None yet

2 participants