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

Feature: Add toolbar environment variables #23

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

Conversation

nakamuraos
Copy link
Contributor

@nakamuraos nakamuraos commented Jan 11, 2021

This PR has the features:

1. Add toolbar environment variables: user can add, edit or remove environment variables

image

How to use?

  • This feature only simple attach one or many command to your command which you typed. For example:
# your environment
export HOME=/home/p0wny
# your command
ls -la

# the command after you hit enter
export HOME=/home/p0wny;la -la
  • You also can add many environment variables. They will be joining by ; (your command will final) after you hit enter.
  • The changes are only for HTML, JS, not change your PHP code.

2. Fix key arrow up

  • When press key arrow up, the cursor will at begin of input. I added a setTimeout() to move cursor to end of input.
setTimeout(function() { eShellCmdInput.focus(); }, 10);

3. Fix CSS logo

  • Your logo is not show on mobile, so I add it again.

4. A little change

  • I just define and replace your code as below:
var $ = function (e) {
    return document.getElementById(e);
}

5. Bonus

  • A new scrollbar homogeneities with the background color.

@nakamuraos nakamuraos changed the title Feature: Add toolbar environtment Feature: Add toolbar environment Jan 11, 2021
@nakamuraos nakamuraos changed the title Feature: Add toolbar environment Feature: Add toolbar environment variables Jan 11, 2021
@flozz
Copy link
Owner

flozz commented Jan 21, 2021

Thank you for your contribution, I will review your PR soon :)

@flozz
Copy link
Owner

flozz commented Jan 24, 2021

Hello,

Thank you again for contributing to p0wny@shell. Here are some some initial review and remarks about the PR


  1. First, there is too much different things in this PR, that makes it more difficult to review. It is better to make one PR for each change:

    • Click on the page to focus the input,
    • Fix the behavior of the ↑ key,
    • CSS fixes (logo and scroll bar)
    • The Env feature

I cherry-picked the 3 first features of the list above as they were mostly ok → it's on master, please update your branch and resolve conflicts ;)


  1. I am not fan of the $ function shortcut, I think it makes the code harder to read, I prefer explicit code.

  2. Be careful of the indentation: the project use 4 spaces and no tabs :)


I will continue reviewing the env feature itself after you rebased the PR on the new master :)

@nakamuraos
Copy link
Contributor Author

Thank @flozz for your review.

Sorry for rules of code. Because this really is first my pull request to contribute on GitHub, so I can haven't experience for this 🗡️

Let me to resolve all this issues

@nakamuraos
Copy link
Contributor Author

Updated as your comment @flozz:

  • Revert $ to document.getElementById for explicit code
  • Fix conflict

@nakamuraos
Copy link
Contributor Author

nakamuraos commented Jan 25, 2021

Have a plan to update env feature better after this merged:

  • Save history and env to localStorage

Besides, I always want this script as simple as possible (as a part of p0wny-shell's goals)

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