Inline Editor Close Button #5443

VizuaaLOG · 2013-10-07T19:15:26Z

I have added a close button to the inline editor as was requested here #5441. The unit test was successful. Please review the work and leave any feedback.

marcelgerber · 2013-10-07T19:43:07Z

Works fine for me, unit tests were successful.

larz0 · 2013-10-09T03:54:30Z

Thanks @VizuaaLOG, looks good.

@njx we should do the same for color editor as well right?

(Tagging @redmunds for the upcoming bezier editor.)

marcelgerber · 2013-10-09T06:12:44Z

If we do this in the color editor, we should also do this in WebDocs.
But I wonder if there is a way to ingegrate this in every inline editor, also if it is called ba an extension. That would maybe be a better solution.

VizuaaLOG · 2013-10-09T20:18:58Z

@larz0 Thanks :).

@SAplayer I could look into if the inline editors have any classes in common and then add it in that way.

njx · 2013-10-09T20:27:54Z

@VizuaaLOG Take a look at InlineWidget (which is the base class for all the inline editors) and see if you can put it in there in a reasonable way.

marcelgerber · 2013-10-09T20:56:25Z

@njx I tried this (b25fd99890ffcae5ecf3ed305a6ed6c78aaa70aa) - the result works for WebDocs, but not for the Inline Editor, because it is would be underneath the scrollbar (not visible). The problem is, we haven't got a default way/element for the InlineWidget. Every (default) extension has its own way to create an InlineWidget.

VizuaaLOG · 2013-10-09T21:29:26Z

@SAplayer @njx I think it will work on all inline widgets. I moved the code into InlineWidgets.js and changed it slightly. I also edited the styles in brackets.less so that it should always be positioned in the same place.

RaymondLim · 2013-10-09T21:40:29Z

src/editor/InlineWidget.js

+        // create the close button
+        var $closeBtn = this.$htmlContent.append("<a href='#'' class='close' id='inline-close'>&times;</a>");
+
+        $closeBtn   .on("mouseup", function(e) {


@VizuaaLOG
Please edit your code in Brackets with the default JSLint on. You will see some JSLint errors on this line telling you to have exactly one space after function and should not have extra white spaces before .on.

VizuaaLOG · 2013-10-09T21:49:33Z

@RaymondLim Fixed the jslint errors.

RaymondLim · 2013-10-09T21:51:51Z

@VizuaaLOG Your close button works, but focus gets lost after clicking on it though. If you start typing after using the close button, nothing shows up in the editor.

RaymondLim · 2013-10-10T07:14:17Z

src/editor/InlineWidget.js

-
+
+        // create the close button
+        var $closeBtn = this.$htmlContent.append("<a href='#'' class='close' id='inline-close'>&times;</a>");


You have an extra single quote after href='#' and this extra quote is causing it to be an invalid element (at least for some jQuery apis). This is why focus gets lost after using the close button.

VizuaaLOG · 2013-10-10T17:37:50Z

I fixed the extra quote problem however the focus is still lost when clicking the close button. I thought about doing editor.focus() however the only way i can do this is by requiring the editor.js file in InlineWidget.js. If you have any other solutions please say. I am still currently looking for another way.

larz0 · 2013-10-10T17:59:37Z

@VizuaaLOG just letting you know we've decided to move the close button to the left so that it can be more consistent with other inline editors and quick doc. Thanks for you help!

VizuaaLOG · 2013-10-10T18:08:15Z

@larz0 Ok, i fixed the focus problem. Also did you want me to commit it on the left or has it already been done?

RaymondLim · 2013-10-10T18:14:16Z

src/editor/InlineWidget.js

-
+
+        // create the close button
+        this.$closeBtn = this.$htmlContent.append("<a href='#' class='close' id='inline-close'>&times;</a>");


This assignment is wrong. You're assigning the entire inline widget as close button. What you want is create an element with your "a" tag and assign it to the close button before appending it to $htmlContent.

Now with this code you can close the inline editor just clicking anywhere inside the inline.

RaymondLim · 2013-10-10T18:19:02Z

@VizuaaLOG Larz is informing you to move the button to the left since you're working on it. So we're waiting for your commit.

larz0 · 2013-10-10T18:22:25Z

@VizuaaLOG yes please :)

VizuaaLOG · 2013-10-10T18:27:18Z

Ok no problem will fix the problem with the entire widget being a close button then will commit the new changes

VizuaaLOG · 2013-10-10T19:19:45Z

Just wondering if someone could help me :). I have changed the close button code to this

// create the close button
var closeBtn = "<a href='#' class='close' id='inline-widget'>&times;</a>";
closeBtn.click(function (e) {
    self.close();
    e.stopImmediatePropagation();
});
this.$htmlContent.append(closeBtn);

However i get the following error in dev tools Uncaught TypeError: Object <a href='#' class='close' id='inline-widget'>×</a> has no method 'click'. Am i missing something or have i misunderstood what @RaymondLim said?

RaymondLim · 2013-10-10T19:35:18Z

You need to wrap your link element with $() and create an jQuery object and also prefix your variable with $ to indicate that it's a jQuery object.

VizuaaLOG · 2013-10-10T19:44:49Z

Thanks it worked :) Just about to commit the changes

marcelgerber · 2013-10-10T19:52:03Z

I think you missunderstood him again (perhaps your method works, but it isn't nice). Maybe take a look at 0523bc6.

RaymondLim · 2013-10-10T19:52:42Z

src/editor/InlineWidget.js

-
+
+        // create the close button
+        var closeBtn = $("<a href='#' class='close' id='inline-close'>&times;</a>");


It's better to keep it as this.$closeBtn instead of a local variable. Also it is important to prefix with a $ if it is a jQuery object and not a regular HTML element.

VizuaaLOG · 2013-10-10T19:56:41Z

@RaymondLim ok will change it now also is @SAplayer's method better because i think it looks cleaner so i will change it to that.

RaymondLim · 2013-10-10T20:03:27Z

Yes, @SAplayer's code is better as you're chaining to the existing code with the new close button first.

RaymondLim · 2013-10-10T20:04:55Z

src/editor/InlineWidget.js

+        // create the close button
+        this.$closeBtn = this.$htmlContent.find("#inline-close");
+        this.$closeBtn.click(function (e) {
+            console.log("Clicked");


No, please don't leave your debugging code in.

Yes sorry i noticed it after i committed it so i'm just undoing it then re committing

VizuaaLOG · 2013-10-10T20:14:54Z

I think that is everything. I had to re commit the style because i undid the commits too far :(. But it all works again.

peterflynn · 2013-10-10T20:25:12Z

src/editor/InlineWidget.js

-            .append("<div class='shadow bottom' />");
-
+            .append("<div class='shadow bottom' />")
+            .append("<a href='#' class='close' id='inline-close'>&times;</a>");


We can't use an id here because there can be multiple inline editors open at once. If you change this to a class it should work fine.

But it seems like you could just use the existing .close class on this node -- the find() call below should still work since it's scoped, and the CSS rule below is already nested under a .inline-widget selector as well.

VizuaaLOG · 2013-10-10T20:43:10Z

I have changed it to use the class and removed the ID and it all still works.

RaymondLim · 2013-10-10T20:47:11Z

src/editor/InlineWidget.js

+            .append("<a href='#' class='close'>&times;</a>");
+
+        // create the close button
+        this.$closeBtn = this.$htmlContent.find('.close');


Nit: Please use double quote for strings as this is our coding convention.

VizuaaLOG · 2013-10-10T20:53:33Z

@RaymondLim Sorry i changed it, need to get used to that because i usually use single quotes.

RaymondLim · 2013-10-10T20:55:06Z

Looks good now. Merging.

VizuaaLOG · 2013-10-10T20:56:44Z

Ok thanks.

Inline Editor Close Button

larz0 · 2013-10-10T21:01:40Z

🍻

RaymondLim · 2013-10-10T21:19:34Z

Oops... Looks like this breaks one of the WebPlatformDocs extensions unit tests. File a new issue #5473 for the unit test failure.

njx · 2013-10-10T21:19:52Z

I just checked this out and it looks fine everywhere except Quick Docs, where it seems to be causing the content in the right column to shift downward. Filed as #5472.

VizuaaLOG · 2013-10-10T21:26:07Z

I just looked at the problem and it is the margin right to move the content away from the close button. However, i cant think of another way to move the content away from the button. Any ideas?

larz0 · 2013-10-10T21:35:42Z

@VizuaaLOG I'll take a look.

VizuaaLOG · 2013-10-10T21:36:42Z

Found the problem and will make a branch to fix it

njx · 2013-10-10T21:37:55Z

See comments in #5472. I don't think it's necessarily the margin's fault--it might be the fact that we're adding a float into the top level.

VizuaaLOG · 2013-10-10T21:43:04Z

i tried getting rid of the float and it didn't change. The fix i had was changing the left columns's width to 26% which fixes the problem. However i'm new to git and am having some teething troubles with it so i cant fix it :). Will get my git fixed tomorrow then try then

larz0 · 2013-10-10T21:49:46Z

@VizuaaLOG sounds good :)

Added close button to inline editor

0f39692

Added the close button for all inline widgets

e331204

RaymondLim reviewed Oct 9, 2013
View reviewed changes

Fixed jslint errors

1bd959f

RaymondLim reviewed Oct 10, 2013
View reviewed changes

ghost assigned RaymondLim Oct 10, 2013

Fixed a typo in code

f4014f3

Editor now gets focus back when clicking the close button

1277679

RaymondLim reviewed Oct 10, 2013
View reviewed changes

Tidied up the code for easier reading

9b10a82

peterflynn reviewed Oct 10, 2013
View reviewed changes

Identifying button with class instead of an id

a0f8a30

RaymondLim reviewed Oct 10, 2013
View reviewed changes

Code cleanup

dfa7cb5

RaymondLim added a commit that referenced this pull request Oct 10, 2013

Merge pull request #5443 from VizuaaLOG/inline-editor-close-button

8b7d501

Inline Editor Close Button

RaymondLim merged commit 8b7d501 into adobe:master Oct 10, 2013

VizuaaLOG deleted the inline-editor-close-button branch October 10, 2013 20:57

njx mentioned this pull request Oct 10, 2013

Right column in Quick Docs is pushed downward #5472

Closed

RaymondLim mentioned this pull request Oct 10, 2013

One of the Quick Doc unit tests is failing. #5473

Closed

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Inline Editor Close Button brackets-archive/bracketsIssues#4998

Open



		// create the close button
		var $closeBtn = this.$htmlContent.append("<a href='#'' class='close' id='inline-close'>×</a>");



		// create the close button
		this.$closeBtn = this.$htmlContent.append("<a href='#' class='close' id='inline-close'>×</a>");



		// create the close button
		var closeBtn = $("<a href='#' class='close' id='inline-close'>×</a>");

Inline Editor Close Button #5443

Inline Editor Close Button #5443

Conversation

VizuaaLOG commented Oct 7, 2013

marcelgerber commented Oct 7, 2013

larz0 commented Oct 9, 2013

marcelgerber commented Oct 9, 2013

VizuaaLOG commented Oct 9, 2013

njx commented Oct 9, 2013

marcelgerber commented Oct 9, 2013

VizuaaLOG commented Oct 9, 2013

Choose a reason for hiding this comment

VizuaaLOG commented Oct 9, 2013

RaymondLim commented Oct 9, 2013

Choose a reason for hiding this comment

VizuaaLOG commented Oct 10, 2013

larz0 commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

Choose a reason for hiding this comment

RaymondLim commented Oct 10, 2013

larz0 commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

RaymondLim commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

marcelgerber commented Oct 10, 2013

Choose a reason for hiding this comment

VizuaaLOG commented Oct 10, 2013

RaymondLim commented Oct 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VizuaaLOG commented Oct 10, 2013

Choose a reason for hiding this comment

VizuaaLOG commented Oct 10, 2013

Choose a reason for hiding this comment

VizuaaLOG commented Oct 10, 2013

RaymondLim commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

larz0 commented Oct 10, 2013

RaymondLim commented Oct 10, 2013

njx commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

larz0 commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

njx commented Oct 10, 2013

VizuaaLOG commented Oct 10, 2013

larz0 commented Oct 10, 2013