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

feat #309 vertical splitter implementation #311

Closed
wants to merge 1 commit into from

Conversation

@Gprasad
Copy link
Contributor

Gprasad commented Jan 7, 2013

feat #309 vertical splitter implementation

@piuccio
piuccio reviewed Jan 7, 2013
View changes
src/aria/widgets/container/Splitter.js Outdated
this._handleBarClass = (cfg.orientation == "horizontal")
? "xSplitter_" + cfg.sclass + "_sHandleH"
: "xSplitter_" + cfg.sclass + "_sHandleV";
this._handleBarClass = orientation ? "xSplitter_" + cfg.sclass + "_sHandleH" : "xSplitter_" + cfg.sclass

This comment has been minimized.

Copy link
@piuccio

piuccio Jan 7, 2013

Contributor

This should be more readable

this._handleBarClass = "xSplitter_" + cfg.sclass + (orientation ? "_sHandleH" : "_sHandleV");

@piuccio
piuccio reviewed Jan 7, 2013
View changes
src/aria/widgets/container/Splitter.js Outdated
this._handleBarClass, ' " style="top:', size.size1,
'px; width:100%;"></span><span class="xSplitter_', cfg.sclass, '_sMacro" style="height: ',
size.size2, 'px;width:', cfg.width, 'px;bottom:0px">'].join(""));
if (orientation) {

This comment has been minimized.

Copy link
@piuccio

piuccio Jan 7, 2013

Contributor

There is some duplication, apparently to handle top/width instead of left/height. Can we use parameters instead?

This comment has been minimized.

Copy link
@Gprasad

Gprasad Jan 8, 2013

Author Contributor

This has been fixed now just using the parameters and removed the duplicate code.

@piuccio
piuccio reviewed Jan 7, 2013
View changes
src/aria/widgets/container/Splitter.js Outdated
@@ -211,58 +260,62 @@ Aria.classDefinition({
* @param {aria.widgets.CfgBeans.SplitterCfg}
*/
_calculateSize : function (cfg) {
var bdr = cfg.border ? this.SPLITTER_BORDER_SIZE : 0;
var bdr = cfg.border ? this.SPLITTER_BORDER_SIZE : 0, size = {}, totalHeight, initDimension;

This comment has been minimized.

Copy link
@piuccio

piuccio Jan 7, 2013

Contributor

Variable names are not expensive, border is better than bdr

var size2 = ((dimension - size1) <= 0) ? 0 : (dimension - size1);
this.setProperty("size1", size1);
this.setProperty("size2", size2);
this._splitPanel1.style[dimType] = size1 + "px";

This comment has been minimized.

Copy link
@piuccio

piuccio Jan 7, 2013

Contributor

You moved this part before the section refresh, is there a reason?
Somehow it seems more logical, but I wonder whether there was a reason to do a refresh first

This comment has been minimized.

Copy link
@Gprasad

Gprasad Jan 8, 2013

Author Contributor

Having the Refresh before the size properties was setting the wrong values and was preventing the implementation of nested splitters.

@piuccio
piuccio reviewed Jan 7, 2013
View changes
test/aria/utils/DatePatternInterpret.js Outdated
@@ -56,9 +56,11 @@ Aria.classDefinition({
this.interpretInpuPatternAndAssert("2012*10;03", date3options, expectedDate);
this.interpretInpuPatternAndAssert("12-juL", date3options, new Date(2012, 6, 1));
this.interpretInpuPatternAndAssert("9/12.7", date3options, new Date(2012, 6, 9));
var currentYear = new Date().getFullYear();

This comment has been minimized.

Copy link
@piuccio

piuccio Jan 7, 2013

Contributor

I've pushed a fix for this test in its own commit. Could you please revert your changes and rebase on master?

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation

feat #309 vertical splitter implementation
@piuccio

This comment has been minimized.

Copy link
Contributor

piuccio commented Jan 10, 2013

Integrated in 2c68fa5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.