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

Project startup items #1

Closed
r4j4h opened this issue Aug 15, 2017 · 6 comments
Closed

Project startup items #1

r4j4h opened this issue Aug 15, 2017 · 6 comments
Assignees

Comments

@r4j4h
Copy link
Contributor

r4j4h commented Aug 15, 2017

Hi there! Thanks for starting this.

In my fork I have started adding a Right-click menu, with corresponding Option to toggle on and off, and in a separate, related branch I started added Keyboard Shortcuts, also with a toggle and ideally a keybind customization menu panel in the future.

I want to open a PR now, but I have some concerns I want to address so that I can clean things up appropriately for a proper PR.

  1. I used SASS. It's light, so I can totally clean up a MR that is SASS-less, but ideally we would want a build chain to convert them to CSS, and that means development dependencies. So I wanted to see how you felt build chains.

  2. New dependencies. In the Keyboard Shortcuts branch I brought in Keypress to help, but I also kept all the usage of the library into one "driver" class so it's fairly easily to incorporate a different solution. Possibly jQuery or mootools based since those are already there, or something else, if you have a preference?

@cxw42
Copy link
Owner

cxw42 commented Aug 15, 2017

My pleasure, and thanks for contributing! Both are good questions that I would like to think about before I respond, since they will set the course for future contributions as well. My development environment is cygwin, but I'm not aware that would restrict the toolchain any. Let me check out Keypress and SASS and then I will have something intelligent to say :) .

Edit I ❤️ keyboard shortcuts

@cxw42 cxw42 assigned r4j4h and cxw42 Aug 15, 2017
@cxw42
Copy link
Owner

cxw42 commented Aug 15, 2017

@r4j4h would you please point me to your build script (or add it here or in the repo if it's not there yet)? It's not jumping out at me, and I would like to see what's involved in your present workflow. Thank you!

@r4j4h
Copy link
Contributor Author

r4j4h commented Aug 16, 2017

Hey @cxw42 sorry you are not going crazy, there isn't one yet! The files that are there are this scss file and this css file.

The only thing actually using sass functionality is the useless background: red class on .context-menu, leading me think I should separate the SASS topic from the other two topics. I started writing the css as SASS out of habit, and for reference lifted the CSS itself from https://github.com/callmenick/Custom-Context-Menu/blob/master/style.css.

That said I do like SASS a lot, to the point of writing this :p

It is pretty powerful http://sass-lang.com/guide but I think the simplest, handiest features for me are variables which let me explain away magic numbers and easy re-inclusion of code, but for those alone LESS or something else is probably just as good and it becomes more about ease of use with IDEs and syntax preferences.

As far as build script, which I guess is topic #4 😊 , webpack is solid, flexible enough to work with almost anything, and has quite a bit of momentum, so I suggest we consider it for building.

if you are familiar with containers or Docker then we can utilize them to prevent needing to install these tools on your machine and can help keep version smear in check. A Virtual Machine would work as well. The idea being to volume mount this repo's directory into the VM or container and to open the files both there and back on the host, running the build chain there and editing the files in the IDE on the host.

@cxw42 cxw42 mentioned this issue Aug 16, 2017
@cxw42
Copy link
Owner

cxw42 commented Aug 16, 2017

Build is indeed now #4 :) .

Context menu is now #6. I tried that one first since it seemed like a smaller bit to test. I will check the keyboard shortcuts later.

SASS seems fine to me. Keypress, likewise - my only concern was that it might interfere with jstree's keyboard nav, but it does not seem to do so in the right-click branch.

From a quick look, I have no objection to webpack. What version are you using? I see the v1 and v2 docs are separate. The one thing I would like to do once we switch over is to force-add /dist to the repo whenever I update the Chrome Store (coming soon!) so that people who want to report bugs can browse the deployed code on github and not have to unpack the extension (which I have done before).

Having given the matter some thought, I would prefer not to use VMs at this point. If nothing else, I don't want to have to keep my bashrc in sync between cygwin and a container! :) Also, I see some search results suggesting that Docker may require local admin on Windows, and I would rather that not be a barrier to entry for other potential contributors. I have been without local admin in the past, and I recall the pain :) .

Separately, let it be known to the world that version numbers will follow semver!

@cxw42 cxw42 changed the title Keyboard Shortcuts & Right Click Menus Project startup items Aug 16, 2017
@r4j4h
Copy link
Contributor Author

r4j4h commented Aug 17, 2017

I didn't think about intereference with jstree's keyboard nav so I am glad you brought that up and that it worked out. For keybinds we should prevent the user from binding over jstree's, or see if we can configure it.. upon initial inspection configuration of shortcuts was removed in v3 as it "follows ARIA spec for trees to enable keyboard navigation for visually impaired people." (see google group thread and github issue)

I use version 2 of webpack where I can. As far as I know for a new project there's no reason to use version 1 anymore. I forgot to mention that I think webpack requires node.. on the positive side npm (which comes with it) would be helpful in installing/maintaining dependencies, especially since webpack is built "modular" so some features require reaching out to a couple of different repositories.

Thanks for calling out the complication the Chrome Store introduces. I'm not familiar with the pre-packaging process, so I'm thankful you have done it before. As far as I understand you I don't think handling /dist in that way should be a problem.

Users not being required to use VMs or containers doesn't preclude them from using them if they want, so I think inclusive is good. For what it's worth though, you can share your .bashrc with your containers too. It is most definitely true that Docker will likely require some Admin level privs though - at the very least Docker Toolbox's use of VirtualBox will when creating it initially. Having limited privs is definitely a pain! Offering a basic development environment in a Dockerfile may still be useful so I'll probably put up a PR for that at some point.

Hooray semver! Sounds good to me. 👍

@r4j4h r4j4h mentioned this issue Aug 17, 2017
1 task
@cxw42
Copy link
Owner

cxw42 commented Aug 17, 2017

  • I've used node and npm before, so no problem there.
  • Also no concerns with an optional dockerfile. How big is the file? If it is jumbo, I would like to put it in a submodule, or put it in a separate repo linked from the README. Maybe a separate repo that has this one as a submodule? Please open another issue or PR whenever you're ready.

I think everything else on the list has been moved to a separate issue, so I'm going to close this. Feel free to ping or reopen if necessary!

@cxw42 cxw42 closed this as completed Aug 17, 2017
cxw42 pushed a commit that referenced this issue Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants