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

Add "close button" #140

Merged
merged 4 commits into from
Jun 22, 2015
Merged

Conversation

sh19910711
Copy link

The main changes of this pull request are the followings:

  • Change styles of the resizer element
  • Change the positioning of the console elements
  • Add "Close button"
  • Add "Minimize button"

Well, the absolute positioning is to add more elements like the resizer element. For example, I think to add "minimize button", "close button" and "setting button" to the console, and the absolute positioning is able to show multiple elements like that to the top of the console element.

The quoted text is from PR #139.

GIF

gif

Issue #121

@sh19910711
Copy link
Author

I have pushed commits as the first version which contains wip-commits for code reviews. Thanks.

@gsamokovarov
Copy link
Collaborator

Because we share the browser space with user content, I think we shouldn't waste that much real estate with the resize bar. We can keep it as is, and provide just the close button. If people want the console out of the way, they can just resize it down. Further more, we can probably drop the outline of the close button and show it only when the mouse hovers the console (so again, we save the real estate).

We also should be able to opt-out of this behaviour for the browser extensions.

@sh19910711
Copy link
Author

@gsamokovarov Thanks for the reply.

Because we share the browser space with user content, I think we shouldn't waste that much real estate with the resize bar.

I understand that.

We can keep it as is, and provide just the close button. If people want the console out of the way, they can just resize it down.

And I also agree with that.

Further more, we can probably drop the outline of the close button and show it only when the mouse hovers the console (so again, we save the real estate).

Uhm, I'm not quite sure of that. The next link shows my interpretation: http://jsfiddle.net/bmvfLkjq/4/

Let me know if it is incorrect.

We also should be able to opt-out of this behaviour for the browser extensions.

Yeah, I think to add a config of the REPLConsole constructor.

Thank you.

@gsamokovarov
Copy link
Collaborator

Uhm, I'm not quite sure of that. The next link shows my interpretation: http://jsfiddle.net/bmvfLkjq/4/

My thought was to keep the resize bar transparent as before. We may not have the close button attached to it (at least visually), having the close boutton couple of pixels down to the right of it may cut it. Something like this ASCII:

+---------------------------+  <- You can assume that this line is
|                         + |     the resize bar :-)
|                           |
|                           |
|                           |
|                           |
+---------------------------+

Yeah, I think to add a config of the REPLConsole constructor.

Exactly.

@sh19910711
Copy link
Author

@gsamokovarov Thanks for giving an example :-) OK, I will think of it.

@sh19910711 sh19910711 force-pushed the patch/0008/add-close-button branch 2 times, most recently from ae0eed9 to 8c55d2d Compare June 21, 2015 05:27
@sh19910711 sh19910711 changed the title Add "Close button" and "Minimize button" Add "close button" Jun 21, 2015
@sh19910711
Copy link
Author

I have pushed new version with the following changes:

  • Remove the minimize button
  • The shiftConsoleActions() is for the fixed positioning

@gsamokovarov Hi, could you review the code again?

And here is a gif:

gif

Thank you.

.console .resizer { background: #333; width: 100%; height: 4px; cursor: ns-resize; }
.console .console-actions { padding-right: 3px; }
.console .console-actions .button { float: left; }
.console .button { cursor: pointer; border-radius: 1px; font-family: monospace; font-size: 9px; width: 10px; height: 10px; line-height: 10px; text-align: center; color: #ccc; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the close button a bit bigger? Say font-size: 13px, width: 14px; height: 14px; line-height: 14px;, I tested on my machine and it looks reasonable.

@gsamokovarov
Copy link
Collaborator

Thanks @sh19910711, this looks great! Now that we have templates tests infrastructure, we can try to write a regression test for this.

@sh19910711 sh19910711 force-pushed the patch/0008/add-close-button branch from 8c55d2d to b354089 Compare June 21, 2015 09:41
@sh19910711
Copy link
Author

Now that we have templates tests infrastructure, we can try to write a regression test for this.

OK, I will try it.

Thanks for taking time to check :)

@sh19910711 sh19910711 force-pushed the patch/0008/add-close-button branch from 0eb72dc to 3407c15 Compare June 21, 2015 12:30
SpecHelper.triggerEvent(closeButton, "click");
});
it("should be removed from the parent node", function() {
assert.ok(document.getElementById(this.elmId) === null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there assert.nil or something in the lines of?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found assert.isNull :)

http://chaijs.com/api/assert/#isNull-section

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use it then :)

@sh19910711 sh19910711 force-pushed the patch/0008/add-close-button branch from 3407c15 to 4ec707a Compare June 21, 2015 13:30
@sh19910711
Copy link
Author

Updated (and here is a diff). The SpecHelper.prepareStageElement() is to do as its name implies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants