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

Assignable Keyboard Shortcuts Updates #3720

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

WebShells
Copy link
Member

Few updates regarding HotKeys/ Keyboard Shortcut section.

. Added Shortcut TAB under config section where user can re-assign the shortcuts as needed
. Shortcuts Help Modal in Sales Register now reflects the assigned shortcuts
. Updates JS section in register in order to retreive assigned keys from database.

To be fixed:
. Migration file
. Success message on submit

Few updates regarding HotKeys/ Keyboard Shortcut section.

. Added Shortcut TAB under config section where user can re-assign the shortcuts as needed
. Shortcuts Help in Sales Register now reflects the assigned shortcuts
. Updates JS section in order to retreive assigned keys from database.

Still need to submit migration file and few fixes.
public function get_key_shortcuts_options()
{
$key_shortcuts = array();
$key_shortcuts['27 | ESC'] = $this->CI->lang->line('sales_key_1');
Copy link
Member

Choose a reason for hiding this comment

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

this can be done in a loop

@jekkos
Copy link
Member

jekkos commented Mar 18, 2023

@WebShells we might want to freeze master branch until the CI4 upgrade is done, merging this now will only create more conflicts later on. Any new functionality should be built on top of CI4.

@WebShells
Copy link
Member Author

@WebShells we might want to freeze master branch until the CI4 upgrade is done, merging this now will only create more conflicts later on. Any new functionality should be built on top of CI4.

@jekkos do you want me to apply the changes to fit to CI4 upgrade and submit the PR to that branch ? or shall i wait till the upgrade is done ?

@objecttothis
Copy link
Member

@WebShells we might want to freeze master branch until the CI4 upgrade is done, merging this now will only create more conflicts later on. Any new functionality should be built on top of CI4.

@jekkos do you want me to apply the changes to fit to CI4 upgrade and submit the PR to that branch ? or shall i wait till the upgrade is done ?

I recommend we still set this PR to merge with the master but write the changes to work with CI4. Then it will be ready to test/merge after the CI4 branch has been merged into the master.

I agree that the master branch needs to be frozen.


public function get_key_shortcuts_options()
{
$key_shortcuts = array();
Copy link
Member

@jekkos jekkos Mar 25, 2023

Choose a reason for hiding this comment

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

What could be cleaner here is to approach this in OOP fashion and have a proper Shortcut class or a central dict, that has fields like keycode, lang_line. Then most of the repetitive parts in this code can be generated dynamically, by filling in the correct variable at every step

</tr>
</thead>
<tbody>
<tr>
<td><code>ESC</code></td>

<td><?php echo $this->lang->line('sales_key_cancel'); ?></td>
Copy link
Member

Choose a reason for hiding this comment

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

here you would have one loop going over the datastructure and printing the rows in generic fashion

@jekkos
Copy link
Member

jekkos commented Mar 25, 2023

@WebShells we might want to freeze master branch until the CI4 upgrade is done, merging this now will only create more conflicts later on. Any new functionality should be built on top of CI4.

@jekkos do you want me to apply the changes to fit to CI4 upgrade and submit the PR to that branch ? or shall i wait till the upgrade is done ?

I recommend we still set this PR to merge with the master but write the changes to work with CI4. Then it will be ready to test/merge after the CI4 branch has been merged into the master.

I agree that the master branch needs to be frozen.

Fine by me, I guess this won't be a big one. But we need a migration at least otherwise there might be issues in the register @WebShells

Added Migration script in order to insert default shortcut keys values into db.
@WebShells
Copy link
Member Author

@WebShells we might want to freeze master branch until the CI4 upgrade is done, merging this now will only create more conflicts later on. Any new functionality should be built on top of CI4.

@jekkos do you want me to apply the changes to fit to CI4 upgrade and submit the PR to that branch ? or shall i wait till the upgrade is done ?

I recommend we still set this PR to merge with the master but write the changes to work with CI4. Then it will be ready to test/merge after the CI4 branch has been merged into the master.
I agree that the master branch needs to be frozen.

Fine by me, I guess this won't be a big one. But we need a migration at least otherwise there might be issues in the register @WebShells

Added

Copy link
Member

@objecttothis objecttothis left a comment

Choose a reason for hiding this comment

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

Super. Once #3592 is merged with the master you'll need to modify these changes to CI4 formatting and then we can approve and merge. If you want to get a head start, you can take a look at these files in the ci4-upgrade branch and change it now.

Renamed ur_PK to ur-PK as listed in locale_helper.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants