Added all components for Border Radius Inline editor #890

feihaozi77 · 2017-11-02T17:07:56Z

Added all components for inlineBorderRadiusEditor

flukeout · 2017-11-03T14:38:09Z

Hey, awesome - this is super cool @feihaozi77 👍 Some thoughts...

When moving the slider, is it possible to change the value when the slider is moving and not just when the slider stops moving?
- I think the oninput event should do the trick
- onchange only fires at the end
Let's not have any decimal spots in the pixel values
- Instead of 12.75px let's just use 13px
- We can set a number of fixed steps for the input range element
After closing the border radius UI, is it possible to clear the selection, currently it ends up like this...

So when you type anything, it replaces the selection immediately.

Anyway, this is great - really excited to see this progress 💯

Of course, I'll take care of all the styling and UI once the functionality is getting closer to our final version.

feihaozi77 · 2017-11-03T22:07:34Z

@flukeout Sure I'll work on the parts you mentioned.

feihaozi77 · 2017-11-04T14:46:03Z

@flukeout made some changes

flukeout · 2017-11-13T21:18:18Z

Thanks for your patience @feihaozi77 - reviewing again now.

flukeout · 2017-11-13T22:24:35Z

This is getting really good! Here is some more feedback...

When adjusting the top-left-corner, is it possible to not also change the all-corners value?

Is it possible to retain the values of the individual corners so they don't reset when you switch to the All corners mode?

I'm imagining a way where the individual and all modes have their own separate "memory". So if you make changes in one, it doesn't affect the other. So, let's say..

I make some changes to individual corners
I switch to all corners mode
The individual corner value sliders are not affected
If i switch back to individual corners mode, we apply those stored values to the CSS

Does it make sense? Let me know! Thanks so far!!

…fecting each other

feihaozi77 · 2017-11-14T00:09:02Z

@flukeout I made the changes you need to seperate the allCornerSlider and the individualSlider, please review when you are avaliable

flukeout · 2017-11-14T00:24:38Z

Awesome, will try it out now.

flukeout · 2017-11-14T00:53:13Z

OK, great! I think that works much better. Some more requests:

Request 1
Is it possible to show the purple editor icon when there isn't a value yet for the CSS rule? I'm thinking of the case when the user has typed..

border-radius: but hasn't started to add any values yet.

I think it would be good to show the icon right then so they could start using the UI right away if they wanted to.

Request 2
Is it possible to show the pixel value after the range slider? Like this...

all corners ==0========== 12px

Once we get these changes in I can make a PR against your branch with styling updates, etc.

👍

feihaozi77 · 2017-11-14T01:16:00Z

@flukeout let me clarify request1, do you mean you want the purple icon to pop up automatically without user clicking the text when user finished typing something like border-radius:

feihaozi77 · 2017-11-14T06:53:03Z

@flukeout I think I'll get your request 2 done by tomorrow but the 1st request seems pretty hard, I've been stucking there for awhile, I looked into the PopoverWidget.js file, the positionPopover function is being called, if I set a break point inside the positionPopover function, the bubble was showed at first when the execution was stopping at the break point, but once I step over the break point the bubble just dispeared. I could not figure out this, maybe some one who are more familliar with codemirror could help

flukeout · 2017-11-14T16:44:10Z

Hey @feihaozi77 - yeah with regard to Request 1 - that's what I had in mind. If it's proving too time consuming, let's not worry about it too much at this point. We could always try to solve it with another effort down the road.

feihaozi77 · 2017-11-15T18:43:00Z

@flukeout request2 completed

flukeout · 2017-11-15T18:45:12Z

Awesome @feihaozi77 - will take a look a little later today!

flukeout · 2017-11-17T18:22:38Z

Looking now.

flukeout · 2017-11-17T19:04:57Z

This is great @feihaozi77! I've got a question: is it possible for the UI to be aware of the units used in the rule? For example, if I have border-radius: 5px 10%; and open the UI, it converts all of the units to px. Is it possible to keep %?

Then, on top of that, maybe we should add a toggle to each value that lets the user choose then unit. Something like this...

top-left-corner ===0======= 10 [px / % / em]

Let me know if that is possible. Thanks!

feihaozi77 · 2017-11-17T20:49:48Z

@flukeout regarding your request "if I have border-radius: 5px 10%; and open the UI, it converts all of the units to px. Is it possible to keep %?", should be able to do that. not sure about the toggle part yet.

feihaozi77 · 2017-11-19T03:02:35Z

@flukeout Added the part for keeping %, em and px units

flukeout · 2017-11-21T22:52:52Z

Testing now @feihaozi77 - thanks!

flukeout · 2017-11-21T23:48:00Z

Hey @feihaozi77, when opening a single-value border-radius, is it possible to also populate that value into all of the corner values? See the screencap below, when I open it, all of the values are set to 0...

feihaozi77 · 2017-11-22T01:12:04Z

@flukeout sure, I will work on that, and right now i'm trying to add the radio buttons for each of the corners to toggle their units

flukeout · 2017-11-22T01:16:43Z

Ooops, sorry accidentally closed and opened. Awesome, thanks for the update, looking forward to it!

feihaozi77 · 2017-12-14T16:06:57Z

@humphd ready for review again

humphd · 2017-12-18T16:25:22Z

src/brackets.js

    require("utils/Resizer");
    require("LiveDevelopment/main");
    require("utils/ColorUtils");
+    require("utils/BorderRadiusUtils");


We shouldn't need to require() a file needed by an extension in the core. Can you not move this into your main.js or the extension?

@humphd Should I remove this line from the brackets.js file?

Yeah, I think so. The only reason to have it here is if you have to have it loaded early for some reason so that other code can access it, and it has to do some kind of initialization. In your case, you should just load it when you need it, where you need it.

In the brackets.js, the colorUtil.js file is also being requried(), That's why I put the borderRadiusUtil.js in here. And I noticed that, if I remove the borderRadiusUtil.js file away from bracket.js, then the BorderRadiusUtil.js will not be loaded in the browser. So basically, if I remove the file from brackets.js, then in my extension, for instance, the InlineBorderRadius.js will not get the BorderRadiusUtils.js because it is using something like this " brackets.getModule("utils/BorderRadiusUtils"); " to load the util files.

Both require() and brackets.getModule() do the same thing, namely, they load a module. However, you use one or the other depending on where the module is defined. For example, in the core of Brackets, you always use require(). In an extension, you use require() when the module you are loading is defined within the extension. However, if you need to load a core module in an extension, you use brackets.getModule().

If you only need this file in your extension code, which you do, you can just use require() in the extension code before it is used.

I got it thank you, so I think instead of using brackets.getModule, I should use require() to do the samething in my extension files, because I'm only getting reference to that one file , it's not nessasary to put the file in the core module. is this correct?

You got it. Let me know if you need help getting it to load properly--sometimes there can be dependency load-order issues with require.

humphd · 2017-12-18T16:26:25Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+    /** Mustache template that forms the bare DOM structure of the UI */
+    var BorderRadiusTemplate = require("text!BorderRadiusEditorTemplate.html");
+    var DEFAULT_BORDER_RADIUS_VALUE = 0;
+    /**


This comment doesn't seem to match what's happening in the function

humphd · 2017-12-18T16:27:33Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+     * @param {!{horizontalOffset: string, verticalOffset: string, blurRadius: string, spreadRadius: string, color: string}} values  Initial set of box-shadow values.
+     * @param {!function(string)} callback  Called whenever values change
+     */
+    function replaceAll(value,str1,str2,ignore){


I don't feel like this is named well. You have a much more complex regex in here than just replacing one string in another. Can you add a comment to explain the regex, and what it's looking for, and also what this really does?

humphd · 2017-12-18T16:28:57Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+    }
+
+    /**
+     * A string or tinycolor object representing the currently selected color


This still hasn't been addressed.

humphd · 2017-12-18T16:29:47Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+
+    BorderRadiusEditor.prototype._setInputValues = function(setFromString) {
+        var values = this.individualValuesWithUnit;
+            if(!this._allCorners){


This code is all indented too far.

humphd · 2017-12-18T16:30:23Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+
+    };
+    BorderRadiusEditor.prototype.setRadioButtons = function(values){
+            //when initializing


Indented too far.

humphd · 2017-12-18T16:31:06Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+    BorderRadiusEditor.prototype.updateRadios=function(corner,unit){
+        if(corner==="tl"){
+            if(unit==="percent"){
+


Remove this line

humphd · 2017-12-18T16:32:37Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+            result+=_array[i];
+        }
+        this._values = result;
+        this._callback(value);


Does this mean you'll call the callback more than once, since _commitChanges is called more than once above? If so, is that expected? It's an odd pattern: usually you'd just call a callback once, and if you need to call it many times, you'd do some kind of event handler instead.

@humphd In this case, the event handler is being implemented in the InlineBorderRadiusEditor.js layer, whenever the UI element(slider) changes, it will trigger the event handler in BorderRadiusEditor.js, then BorderRadiusEditor.js will update the UI and then send control back to InlineBoderRadiusEditor.js by calling the _callback(), which is mapping to the _handleBorderRadiusChange() in the InlineBorderRadius.js. the _handleBorderRadiusChange() will be updating document text.

@humphd basically, in my code, the InlineBorderRadiusEditor.js handles the Document updating tasks while BorderRadiusEditor.js handles UI Updating first then send control to InlineBorderRadiusEditor.js to update the Text in the hostEditor.

@humphd I'm not really sure how to implement the event handler for the _callback, since BorderRadiusEditor and InlineBorderRadiusEditor works on different layers, could you give more hint on this? or maybe we can discuss about this in class tomorrow.

humphd · 2017-12-18T16:33:14Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditorTemplate.html

+    <header>
+        <ul class="button-bar">
+          <li id="allCorners" title="All Corners">
+              <a href="#" tabindex="0">All Corners</a>


We'll need to localize all user visible strings.

@flukeout @humphd I noticed there are so many files in the src/nls folder for storing all localized strings. Since I need localize all those user visible strings, can anyone tell me where I should save those string key value pairs to?

humphd · 2017-12-18T16:34:01Z

src/extensions/default/InlineBorderRadiusEditor/css/main.less

@@ -0,0 +1,412 @@
+@sansFontFamily: "Open Sans" , Helvetica, Arial, sans-serif;


We need to confirm with @flukeout the font choice here so it matches what we use elsewhere.

humphd · 2017-12-19T16:45:42Z

For localization see https://github.com/mozilla/thimble.mozilla.org/wiki/Localization and https://github.com/mozilla/brackets/wiki/Localizing-Bramble

humphd · 2017-12-19T16:47:00Z

src/brackets.js

    require("utils/Resizer");
    require("LiveDevelopment/main");
    require("utils/ColorUtils");
+    //require("utils/BorderRadiusUtils");


Remove this

humphd · 2017-12-19T16:47:46Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+        return parsedResult;
+    }
+
+    function BorderRadiusEditor($parent, values, callback) {


Rename callback to indicate that it's a handler

feihaozi77 · 2017-12-19T22:01:55Z

@humphd @gideonthomas I made a Pull Request to the thimble repo for adding the localized strings to the locales folder in thimble repe. Please take a look and let me know if that's OK. Here is the PR link mozilla/thimble.mozilla.org#2585

feihaozi77 · 2017-12-20T16:07:36Z

@humphd @gideonthomas ready for review!

gideonthomas · 2018-01-04T17:54:33Z

@feihaozi77, sorry about the delayed response...I haven't forgotten about your PR. Since this is a pretty big code change, I'm going to open a PR against your branch that you can merge in with changes that I suggest.

gideonthomas

Hi @feihaozi77, I have opened up a PR against your branch as you can see in feihaozi77#3. Please read the comment there.

feihaozi77 · 2018-01-04T23:03:14Z

@gideonthomas I have merged your branch. Good work!! I like the idea that you implement the BorderRadiusValue class which makes the code much cleaner and reusable. Thank you very much!!

gideonthomas

added a few more comments @feihaozi77

gideonthomas · 2018-01-05T03:35:33Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+
+        this.allCorners = values.length === 1;
+
+        if (!this.allCorners) {


I noticed another bug. Change this entire if into:

if (!this.allCorners) { secondValue = values[1]; if (numOfValues === 2) { fourthValue = secondValue; } else { thirdValue = values[2]; if (numOfValues === 3) { fourthValue = secondValue; } else { fourthValue = values[3]; } } }

gideonthomas · 2018-01-05T03:35:48Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+            secondValue.unit,
+            this.onChange
+        );
+        this.bottomLeft = new BorderRadiusValue(


this needs to be this.bottomRight

gideonthomas · 2018-01-05T03:36:02Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+        );
+        this.bottomLeft = new BorderRadiusValue(
+            this.$element,
+            "bottom-left",


needs to be "bottom-right"

gideonthomas · 2018-01-05T03:36:15Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+            thirdValue.unit,
+            this.onChange
+        );
+        this.bottomRight = new BorderRadiusValue(


this should be this.bottomLeft

gideonthomas · 2018-01-05T03:36:34Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+        );
+        this.bottomRight = new BorderRadiusValue(
+            this.$element,
+            "bottom-right",


change this to bottom-left

gideonthomas · 2018-01-05T03:40:48Z

src/extensions/default/InlineBorderRadiusEditor/InlineBorderRadiusEditor.js

+    /** @type {boolean} True while we're syncing a color picker change into the code editor */
+    InlineBorderRadiusEditor.prototype._isOwnChange = null;
+
+    /** @type {boolean} True while we're syncing a code editor change into the color picker */


update this comment

gideonthomas · 2018-01-05T03:41:09Z

src/extensions/default/InlineBorderRadiusEditor/InlineBorderRadiusEditor.js

+
+
+    /**
+     * Returns the current text range of the color we're attached to, or null if


update this comment

gideonthomas · 2018-01-05T03:43:58Z

src/extensions/default/InlineBorderRadiusEditor/main.js

+        }
+    }
+
+    function queryInlineBorderRadiusEditorPrivoder(hostEditor, pos) {


gideonthomas · 2018-01-05T03:44:06Z

src/extensions/default/InlineBorderRadiusEditor/main.js

+
+    // Initialize extension
+    ExtensionUtils.loadStyleSheet(module, "css/main.less");
+    EditorManager.registerInlineEditProvider(inlineBorderRadiusEditorProvider, queryInlineBorderRadiusEditorPrivoder);


gideonthomas · 2018-01-05T03:44:23Z

src/extensions/default/InlineBorderRadiusEditor/main.js

+            return true;
+        }
+
+        // Get the css property name after removing spaces and ":" so that we can check for it in the file ColorProperties.json


update this comment

feihaozi77 · 2018-01-05T19:37:07Z

@gideonthomas changes are made!

gideonthomas

needs one more indentation change, otherwise this is good to go.

gideonthomas · 2018-01-08T17:43:39Z

src/extensions/default/InlineBorderRadiusEditor/BorderRadiusEditor.js

+            secondValue = values[1];
+
+            if (numOfValues === 2) {
+              fourthValue = secondValue;


fix the indentation here. Needs to be 4 spaces.

gideonthomas · 2018-01-09T20:11:32Z

Thanks for making the changes @feihaozi77, this looks good now!

fixed

Added all components for Border Radius Inline editor

320f414

remove text selection after closing inline border radius editor

65ba628

made allCornerSlider and individual slider work seperately without ef…

df38f6a

…fecting each other

added the slider value indicator

183704c

feihaozi77 added 2 commits November 18, 2017 21:45

modified BorderRadiusEditor to allow keeping all units(%,em,px)

63f7f4e

modified BorderRadiusEditor to allow keeping all units(%,em,px)--#2

f964e3b

flukeout closed this Nov 22, 2017

flukeout reopened this Nov 22, 2017

added radio buttons to toggle slider unit

c6dcd4a

humphd previously requested changes Dec 18, 2017

View reviewed changes

feihaozi77 added 3 commits December 18, 2017 13:55

fixed indentation and replaceAll() local function

f4792c9

fixed some indentations

82677fa

removed _values local property from borderRadiusEditor.js

43bc8cc

humphd reviewed Dec 19, 2017

View reviewed changes

humphd requested a review from gideonthomas December 19, 2017 16:51

feihaozi77 added 3 commits December 19, 2017 17:31

Fixed few indentations and comments

db8e5ae

added comment for borderRadiusUtil

f42ceb1

added more comments in borderRadiusUtils.js

063d61f

Fixed few comments

4c68e51

Review fixes

de372ac

gideonthomas suggested changes Jan 4, 2018

View reviewed changes

Merge branch 'gideonthomas-border-radius'

aa58423

gideonthomas suggested changes Jan 5, 2018

View reviewed changes

Fixed some codes according to comments from gide

9095909

gideonthomas suggested changes Jan 8, 2018

View reviewed changes

feihaozi77 added 2 commits January 9, 2018 13:03

fixed indentation

1bcd535

fixed indentation 2x

c823dd7

gideonthomas approved these changes Jan 9, 2018

View reviewed changes

gideonthomas merged commit 127fc17 into mozilla:master Jan 9, 2018

		@@ -0,0 +1,412 @@
		@sansFontFamily: "Open Sans" , Helvetica, Arial, sans-serif;


		this.allCorners = values.length === 1;

		if (!this.allCorners) {



		/**
		* Returns the current text range of the color we're attached to, or null if

Added all components for Border Radius Inline editor #890

Added all components for Border Radius Inline editor #890

Uh oh!

Conversation

feihaozi77 commented Nov 2, 2017

Uh oh!

flukeout commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feihaozi77 commented Nov 3, 2017

Uh oh!

feihaozi77 commented Nov 4, 2017

Uh oh!

flukeout commented Nov 13, 2017

Uh oh!

flukeout commented Nov 13, 2017

Uh oh!

feihaozi77 commented Nov 14, 2017

Uh oh!

flukeout commented Nov 14, 2017

Uh oh!

flukeout commented Nov 14, 2017

Uh oh!

feihaozi77 commented Nov 14, 2017

Uh oh!

feihaozi77 commented Nov 14, 2017

Uh oh!

flukeout commented Nov 14, 2017

Uh oh!

feihaozi77 commented Nov 15, 2017

Uh oh!

flukeout commented Nov 15, 2017

Uh oh!

flukeout commented Nov 17, 2017

Uh oh!

flukeout commented Nov 17, 2017

Uh oh!

feihaozi77 commented Nov 17, 2017

Uh oh!

feihaozi77 commented Nov 19, 2017

Uh oh!

flukeout commented Nov 21, 2017

Uh oh!

flukeout commented Nov 21, 2017

Uh oh!

feihaozi77 commented Nov 22, 2017

Uh oh!

flukeout commented Nov 22, 2017

Uh oh!

feihaozi77 commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

flukeout commented Nov 3, 2017 •

edited

Loading

feihaozi77 commented Dec 19, 2017 •

edited

Loading