Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Add setting to toggle keypad layout between numeric & standard #3609

Merged
merged 2 commits into from
Oct 13, 2024

Conversation

p42rthicle
Copy link
Contributor

Pull request (PR) checklist

Please check if your pull request fulfills the following requirements:

  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read and understand the Developer Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • My PR includes only the necessary changes to fix the issue (i.e., no unnecessary files or lines of code are changed).
  • 🎬 I've attached a screen recording of using the new code to the next paragraph (if applicable).

Screen recording of your changes (if applicable):

standard_keypad_layout_setting.mp4

What's changed?

We just had the calculator like numeric pad in the keypad or the AmountKeyboard. No you can toggle the setting in settings and switch between standard and numeric style keypad.

Describe with a few bullets what's new:

  • I've added setting to toggle keypad layout between numeric & standard

Risk factors

  • Not a dangerous feature.
  • Code wise, adding the state directly inside the composable might be an anti pattern but its fine for a simple static composable.
    • The setting is only togglable from the settings so frequent recompositions isn't a issue when the keyboard is already drawn on screen.
    • This way,AmountKeyboard is self-contained, making it easy to reuse without passing parameters.
    • Layout changes are rare with the keyboard rendering, so recomposition has minimal impact on performance.

What may go wrong if we merge your PR?

  • Not a dangerous commit

In what cases won't your code work?

  • N/A

Does this PR close any GitHub issues?

Troubleshooting GitHub Actions (CI) failures ❌

Pull request checks failing? Read our CI Troubleshooting guide.

**/

val context = LocalContext.current
val features = EntryPointAccessors.fromApplication(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this must be remember {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also break previews and screenshot tests but such doesn't seem to exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing I didn't notice it breaking previews, and yes my bad it should be remember

@ILIYANGERMANOV
Copy link
Collaborator

Overall LGTM - just add remember the DI instance creation

@ILIYANGERMANOV ILIYANGERMANOV added the requested changes Changes are needed. Please, apply them label Oct 13, 2024
@nvllz
Copy link
Contributor

nvllz commented Oct 13, 2024

@p42rthicle @ILIYANGERMANOV How about making it a one-selection option? I'd like to see something like a new bottom menu dialog on Tap, like on the transaction entry screen:
f5b057a6-abe1-416a-ae69-56b4969600c0

You could remove the switch and make the tab clickable, which would trigger a bottom menu like the one shown above, where we could choose between numeric and default layouts. If you're able to implement this properly, it would greatly improve the possibilities for this screen.

@ILIYANGERMANOV
Copy link
Collaborator

@p42rthicle @ILIYANGERMANOV How about making it a one-selection option? I'd like to see something like a new bottom menu dialog on Tap, like on the transaction entry screen: f5b057a6-abe1-416a-ae69-56b4969600c0

You could remove the switch and make the tab clickable, which would trigger a bottom menu like the one shown above, where we could choose between numeric and default layouts. If you're able to implement this properly, it would greatly improve the possibilities for this screen.

That's a good suggestion, we can extend IvyFeatures to support EnumFeature and not only BoolFeature

@p42rthicle
Copy link
Contributor Author

@nvllz Do we have a we have a tab next to the keypad where we can select the type?

@nvllz
Copy link
Contributor

nvllz commented Oct 13, 2024

@nvllz Do we have a we have a tab next to the keypad where we can select the type?

Idk what you mean here. I suggested transforming this setting to enum list to reflect something like the transaction type selection. It would allow users to choose between the default and numeric layout, which is just more natural for this type of setting. Now it's just less intuitive. It would require some additional work on your part, but would also allow others to use your code and implement other selection-based tweaks in the ivyfeatures screen more easily, thus further developing the app. Just an idea, if you're not up to it, I'll create a feature request ticket and maybe someone else will pick it up after a successful merge of this PR.

@p42rthicle
Copy link
Contributor Author

Thanks for clarifying, I understand that we want to have an enum in place rather than a simple toggle for easy understanding for the user. What I wanted to ask was do we keep the setting to tweak this under the advanced features in the setting (where it is currently) or do we move this setting to the keypad layout itself where we have an icon upon clicking which we show the 2 option which the user can chose from). This way they can easily toggle this without going into settings. If so, I will pick this up as this won't be a major change. And is easily doable. Thanks!

@ILIYANGERMANOV ILIYANGERMANOV merged commit 3f1db2f into Ivy-Apps:main Oct 13, 2024
9 checks passed
@nvllz
Copy link
Contributor

nvllz commented Oct 13, 2024

What I wanted to ask was do we keep the setting to tweak this under the advanced features in the setting (where it is currently) or do we move this setting to the keypad layout itself [...]

It's up to @ILIYANGERMANOV, but in my opinion keeping this setting here would be better. Thanks for your work!

@p42rthicle p42rthicle deleted the fix-issue-3598 branch October 14, 2024 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
requested changes Changes are needed. Please, apply them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Option to reverse number pad (similar to phone dialer)
3 participants