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

noVNC-style vertical menu #330

Merged
merged 1 commit into from
Nov 23, 2024
Merged

Conversation

corbinwunderlich
Copy link
Contributor

@corbinwunderlich corbinwunderlich commented Nov 20, 2024

TODO:

  • Make it an option and fix the hardcoding
  • Make it less opinionated (right now it just looks like how I want it...)
  • Button to make the menu pop out (optional)
  • Fix formatting (I use 4 indents and Xpra uses 2)

I could use some suggestions for a proper name for it other than "noVNC style menu".

@corbinwunderlich
Copy link
Contributor Author

Don't worry about the commit names, I will go back and squash when I'm done

@corbinwunderlich
Copy link
Contributor Author

What would be the best way to add this other type of menu? Should I hack together something in HTML or just give up and make a different element for this style of menu? I don't know

@totaam
Copy link
Collaborator

totaam commented Nov 21, 2024

What would be the best way to add this other type of menu? Should I hack together something in HTML or just give up and make a different element for this style of menu? I don't know

I have no opinion on this. And if I did, it would not be worth much anyway!
Whatever you think is best.

@corbinwunderlich corbinwunderlich marked this pull request as ready for review November 22, 2024 00:19
@corbinwunderlich corbinwunderlich marked this pull request as draft November 22, 2024 00:19
@corbinwunderlich
Copy link
Contributor Author

I'm sorry @totaam , but eslint is erroring out for me. If you could checkout the branch and just run the formatting for me, that would be of great help. Other than that, I think I finished all the implementations of everything, just check over it. Thanks!

@corbinwunderlich corbinwunderlich marked this pull request as ready for review November 22, 2024 00:53
@corbinwunderlich corbinwunderlich changed the title DRAFT: noVNC-style vertical menu noVNC-style vertical menu Nov 22, 2024
@totaam
Copy link
Collaborator

totaam commented Nov 22, 2024

Let's completely ignore whatever mess eslint was (going to be) suggesting.
In fact, can you undo the formatting changes from your commit?
For a start, it will make it much easier to review.

@corbinwunderlich corbinwunderlich force-pushed the master branch 3 times, most recently from a95b8b8 to 299c991 Compare November 22, 2024 16:30
@corbinwunderlich
Copy link
Contributor Author

Let's completely ignore whatever mess eslint was (going to be) suggesting. In fact, can you undo the formatting changes from your commit? For a start, it will make it much easier to review.

I tried to do as best of a job I could, but it still does have significant diffs from the old commits (probably just due to moved lines from the formatter I used). I could fix it further if you want

@totaam
Copy link
Collaborator

totaam commented Nov 22, 2024

I could fix it further if you want

Yes, please.
Codestyle changes shouldn't be mixed with real useful changes.

@corbinwunderlich
Copy link
Contributor Author

Should be all good now @totaam

@corbinwunderlich
Copy link
Contributor Author

On a side note, it seems that prettier is the formatter xpra-html5 uses. Prettier should either be removed or run once in a while, since it appears it hasn't been run for a long time. Not a huge issue though.

@totaam totaam merged commit 21e283c into Xpra-org:master Nov 23, 2024
1 check passed
@totaam
Copy link
Collaborator

totaam commented Nov 23, 2024

Thanks!

@totaam
Copy link
Collaborator

totaam commented Nov 23, 2024

Prettier should either be removed

Yes, I would love to do that - I've had to undo some of the obnoxious changes it made.

The question is, where?

@corbinwunderlich
Copy link
Contributor Author

corbinwunderlich commented Nov 23, 2024

Prettier should either be removed

Yes, I would love to do that - I've had to undo some of the obnoxious changes it made.

The question is, where?

I'm pretty sure it is due to the git hooks on the husky npm package. It seems that the npm build system really doesn't serve any purpose, so I am pretty sure the entire thing can be removed. If that is not doable, just remove the hook that runs prettier. I have no experience with nodejs, but it should probably be in package.json.

I think it would be beneficial for this project to have a formatter, although it definitely isn't necessary. However, prettier should either be configured to be less newline-obsessed or a different one should be found. Although, I think that it would be better for prettier to be removed than to have it doing whatever it is doing right now

@totaam
Copy link
Collaborator

totaam commented Nov 23, 2024

so I am pretty sure the entire thing can be removed

I would love to do that! It only causes problems: #306 (comment)
But how?

@corbinwunderlich
Copy link
Contributor Author

so I am pretty sure the entire thing can be removed

I would love to do that! It only causes problems: #306 (comment) But how?

We should probably move this to a discussion or issue instead of a closed PR, but when I said to "remove the whole thing," I meant that NodeJS in it's entirety could probably be removed. I don't know if there is something that people actually use inside of it, but afaik it only makes things harder.

@corbinwunderlich
Copy link
Contributor Author

I have found a pretty configurable and non-intrusive html and JS formatter, however, like all formatters, it is a compromise. A weak and lenient formatter like this one does not make a line break after a few tokens, but it cannot enforce a strict code style. An intrusive formatter like prettier would enforce a strict code style, but prettier is very unconfigurable and has a really annoying tendency to create newlines when they are not necessary.

I think I agree with you when you said to remove everything node related. This project is not in need of a system like that

@totaam
Copy link
Collaborator

totaam commented Nov 24, 2024

but it cannot enforce a strict code style

I think that's a positive thing!

prettier is very unconfigurable and has a really annoying tendency to create newlines when they are not necessary

Hear hear!
9e30e97, 020e33a, 4de32bd, 76a406d, bf75a50, f071b44, cffed14, 2d2a19a

@corbinwunderlich
Copy link
Contributor Author

corbinwunderlich commented Nov 24, 2024

but it cannot enforce a strict code style

I think that's a positive thing!

prettier is very unconfigurable and has a really annoying tendency to create newlines when they are not necessary

Hear hear! 9e30e97, 020e33a, 4de32bd, 76a406d, bf75a50, f071b44, cffed14, 2d2a19a

Alright, the one I was using was https://github.com/beautifier/js-beautify. We could still use node (not a huge fan, a lot of boilerplate and unnecessary files for nothing) or we could use nix or something.

For nix, a shell.nix would look like

let
  nixpkgs = fetchTarball "https://github.com/NixOS/nixpkgs/tarball/nixos-24.05";
  pkgs = import nixpkgs { };
in
pkgs.mkShell {
  nativeBuildInputs = with pkgs; [ jsbeautifier ];
}

However, nix is kind of annoying to install on windows (less annoying on anything else, but still inconvenient if you aren't using nixOS like I am). So maybe node is better in that regard? I don't know, I don't use node

A demo for the formatter is https://beautifier.io/

@totaam
Copy link
Collaborator

totaam commented Nov 24, 2024

I very much dislike the cost of running nodejs and the complexity involved.
I don't use MS Windows as a development platform, so that doesn't matter.
I would prefer anything that can be installed from the default Fedora repos - not sure what they have.
Alternatively, anything that doesn't bring a gazillion dependencies with it.
Something like ruff for Python.

@corbinwunderlich
Copy link
Contributor Author

I very much dislike the cost of running nodejs and the complexity involved. I don't use MS Windows as a development platform, so that doesn't matter. I would prefer anything that can be installed from the default Fedora repos - not sure what they have. Alternatively, anything that doesn't bring a gazillion dependencies with it. Something like ruff for Python.

Right. Fedora's repos weridly package a perl rewrite of js-beautifier, but not the actual thing. I don't know if the perl version has a cli, or just a module. Nix can be installed on fedora. If I am not wrong, it has a few deps and is roughly 500mb.

I think the right course of action is to start with removing node, and if needed, another formatter can be added, but only if it is needed.

@corbinwunderlich
Copy link
Contributor Author

@totaam If you would like a formatter, I believe I have come to a fairly satisfactory solution.

  • All of NodeJS is deleted
  • Instead of adding NodeJS in shell.nix, js-beautify is added instead
  • .jsbeautifyrc is created to declare formatter options
  • formatter is (I believe) relatively non-intrusive and does not make any draconian changes, but instead smooths out inconsistencies.

I have gotten it working, so I could make a draft pr if you want. I would probably need to edit the README to add in that to contribute, run nix-shell, and inside the shell, run js-beautify -r [any files you edited]. I could also add to use the determinate nix installer to easily install nix on anything with one command

@corbinwunderlich
Copy link
Contributor Author

corbinwunderlich commented Nov 25, 2024

I could also add a format.sh shell script. Adding the shell script would mean that no commands would need to be listed in the readme, as I could make the script install nix, enter the shell, and run the format command

@totaam
Copy link
Collaborator

totaam commented Nov 27, 2024

All of NodeJS is deleted

I'm sold.

@corbinwunderlich
Copy link
Contributor Author

All of NodeJS is deleted

I'm sold.

#332

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.

2 participants