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.

Sign up for GitHub

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

Jump to bottom

Improve white space on terminal application #36

Closed
heowc opened this issue Nov 12, 2018 · 8 comments
Closed

Improve white space on terminal application #36

heowc opened this issue Nov 12, 2018 · 8 comments
Assignees
@rafaelcamargo
Labels
enhancement New feature or request

Comments

@heowc
Copy link

heowc commented Nov 12, 2018 •
edited
Loading

hello, @rafaelcamargo

Thanks for creating glorious-demo.

I used glorious-demo on my blog.

And I found a problem. There is no problem with editor type, but terminal type does not represent white space.

I used a temporary <span style =' white-space: pre '> ... </ span> to solve this problem. But I think there is a better way.

example

<html>
<head>
    <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@glorious/demo@0.7.0/dist/gdemo.min.css">
</head>
<body>
<div id="container"></div>

<script src="https://cdn.jsdelivr.net/npm/@glorious/demo@0.7.0/dist/gdemo.min.js"></script>
<script>
const demo = new GDemo('#container');

const code = `
function greet(){
    console.log("Hello World!");
}

greet();
`;

demo
    .openApp('terminal', {minHeight: '350px', promptString: '$'})
    .command('cat demo.js', {onCompleteDelay: 500})
    .respond(code)
    .command('')
    .end();

</script>
</body>
</html>

result

$ cat demo.js
function greet(){
console.log("Hello World!");
}

greet();

+) I will make hexo tag library of glorious-demo. how about you?
@rafaelcamargo
Copy link
Member

rafaelcamargo commented Nov 12, 2018 •
edited
Loading

Interesting point @heowc

I have never noticed that, but the implementation is really easy to get done. I have just planned it and, asap, I'll try to release that 👍

Regarding Hexo, I'll be very happy in seeing your lib 😃

@rafaelcamargo rafaelcamargo added the enhancement New feature or request label Nov 12, 2018
@heowc
Copy link
Author

heowc commented Nov 12, 2018 •
edited
Loading

Thanks a lot. 👍

@mostafaebrahimi
Copy link
Contributor

@rafaelcamargo I guess you must add white-space pre !important to .terminal-response-line-text in src\styles\terminal-response-line.styl.
But there is one problem and that is different between code response and terminal response.(we expect white-space pre in code samples but not in terminal responses).
So I think it's better to be selected by client [like @heowc :)] or implement two types of responses

@rafaelcamargo
Copy link
Member

Well noticed @mostafaebrahimi!

My strategy would basically be:
1 - I prefer not to use !important. I consider it a violation of the Open Close principle.
2 - I was thinking to consider white-space: pre only for plain text responses. I would not consider it for html responses, since the client should be 100% free when he decides to respond with a HTML string.

What d'you think about this?

@mostafaebrahimi
Copy link
Contributor

@rafaelcamargo I think it's a better way.
My opinion is to some changes in components\terminal-response-line\terminal-response-line.js

1 - change

function appendText(container, text){
  container.innerText = text;
  container.classList.add('terminal-response-plain-text')
}

3 - change

function appendHtml(container, html){
  container.appendChild(html);
  container.classList.add('terminal-response-html-text')
}

What do you think about these changes?

@heowc
Copy link
Author

heowc commented Nov 13, 2018 •
edited
Loading

Me too! I don't use !important.

@rafaelcamargo
Copy link
Member

@mostafaebrahimi The general idea would be exactly that. 👍
I would just avoid repeating classList.add:

function appendText(container, text){
  container.innerText = text;
  addElementCssClass(container, 'terminal-response-plain-text');
}

function appendHtml(container, html){
  container.appendChild(html);
  addElementCssClass(container, 'terminal-response-html-text');
}

function addElementCssClass(element, cssClass){
  element.classList.add(cssClass);
}

@mostafaebrahimi
Copy link
Contributor

@rafaelcamargo Great 💯

@rafaelcamargo rafaelcamargo mentioned this issue Nov 15, 2018
Open
@rafaelcamargo rafaelcamargo mentioned this issue Nov 18, 2018
Merged
@rafaelcamargo rafaelcamargo self-assigned this Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rafaelcamargo rafaelcamargo

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rafaelcamargo @mostafaebrahimi @heowc