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

Mobile view for sensors #37

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Mobile view for sensors #37

wants to merge 2 commits into from

Conversation

Meliox
Copy link
Owner

@Meliox Meliox commented May 12, 2024

Implements #32

@Meliox Meliox added the enhancement New feature or request label May 17, 2024
@Meliox Meliox changed the title (feat) Node mobile view for sensors Mobile view for sensors May 17, 2024
@Meliox
Copy link
Owner Author

Meliox commented May 17, 2024

@eremem: So upon investigation all code we add in /usr/share/pve-manager/js/pvemanagerlib.js also needs to go into /usr/share/perl5/PVE/API2/Nodes.pm. One way to just to duplicate the code in the script, another is to make bash functions that can be called when writing to the respective files (if it's possible in bash). Another is to create a custom js file and call respective code from the respective files.

First suggest is absolutely the easiest, though a little annoying to maintain. Any thoughts?

@eremem
Copy link
Collaborator

eremem commented May 17, 2024

@eremem: So upon investigation all code we add in /usr/share/pve-manager/js/pvemanagerlib.js also needs to go into /usr/share/perl5/PVE/API2/Nodes.pm. One way to just to duplicate the code in the script, another is to make bash functions that can be called when writing to the respective files (if it's possible in bash). Another is to create a custom js file and call respective code from the respective files.

First suggest is absolutely the easiest, though a little annoying to maintain. Any thoughts?

With /usr/share/perl5/PVE/API2/Nodes.pm do you mean /usr/share/pve-manager/touch/pvemanager-mobile.js?

I had a feeling we'd have to reuse the code in some way after I skimmed through the provided patch :/

As to including JS files ExtJS is quite antiquated - a script loader class will have to be used and include the external code in runtime. The idea with bash functions (maybe returning partial sed scripts for the resulting JS code) appeals to me more.

I'm barely familiar with Sencha Touch, but as far as I remember there are some differences in the declarative syntax. Is the framework-related code surrounding our code really identical to the desktop version?

Anyway, the idea of supporting a mobile version seems to require a lot of extra effort. It would be good to know how big the demand actually is. Maybe some kind of a poll in the Proxmox forum?

@Meliox
Copy link
Owner Author

Meliox commented May 17, 2024

With /usr/share/perl5/PVE/API2/Nodes.pm do you mean /usr/share/pve-manager/touch/pvemanager-mobile.js?

Yes, copy-paste error.

As to including JS files ExtJS is quite antiquated - a script loader class will have to be used and include the external code in runtime. The idea with bash functions (maybe returning partial sed scripts for the resulting JS code) appeals to me more.

Ok.

I'm barely familiar with Sencha Touch, but as far as I remember there are some differences in the declarative syntax. Is the framework-related code surrounding our code really identical to the desktop version?
I can try a text comparison to be sure.

Anyway, the idea of supporting a mobile version seems to require a lot of extra effort. It would be good to know how big the demand actually is. Maybe some kind of a poll in the Proxmox forum?
Maybe, but so far code seems very similar, so should be quite easy to do. I already have it working, just need to get the 'sed' in place.

@eremem
Copy link
Collaborator

eremem commented May 17, 2024

Anyway, the idea of supporting a mobile version seems to require a lot of extra effort. It would be good to know how big the demand actually is. Maybe some kind of a poll in the Proxmox forum? Maybe, but so far code seems very similar, so should be quite easy to do. I already have it working, just need to get the 'sed' in place.

What bothers me is not this current case but that we would need a solution to avoid writing new code twice. But I assume that we both see this challenge quite similarly :)

@Meliox
Copy link
Owner Author

Meliox commented May 17, 2024

Yes, exactly. It also bothers me :)

@Meliox Meliox marked this pull request as draft June 12, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants