Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Basic UI: several fix for selection and switch with mappings #3368

Merged
merged 1 commit into from
May 5, 2017
Merged

Basic UI: several fix for selection and switch with mappings #3368

merged 1 commit into from
May 5, 2017

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented May 4, 2017

Fix #3143
Fix #3346
Fix #3349

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo
Copy link
Contributor Author

lolodomo commented May 4, 2017

@resetnow : that is for you ;)

@sjsf sjsf added the UI-BasicUI label May 4, 2017
Copy link
Contributor

@vlad-ivanov-name vlad-ivanov-name left a comment

Choose a reason for hiding this comment

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

Comments are numbered so it's easier to track context.

@@ -478,12 +479,11 @@
item: _t.item,
value: value
}));
_t.suppressUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

[1] I understand that you need to obtain some properties from server so you don't want to suppress the callback entirely...

Copy link
Contributor

Choose a reason for hiding this comment

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

[6] _t.suppressReset = true

@@ -494,12 +494,16 @@
_t.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

[2] ...but could you please implement another way to suppress the useless call to reset()?

Copy link
Contributor

Choose a reason for hiding this comment

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

[4]

if (_t.suppressReset) {
	_t.suppressReset = false;
	return;
} else {
	_t.reset();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

[5] This has to do with animations and avoiding touching DOM too much

@@ -455,7 +455,8 @@
var
_t = this;

_t.value = _t.parentNode.querySelector(o.formValue);
_t.hasValue = _t.parentNode.getAttribute("data-has-value") === "true";
_t.value = _t.parentNode.parentNode.querySelector(o.formValue);
_t.count = _t.parentNode.getAttribute("data-count") * 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

[3] For example, _t.suppressReset = false here.

@lolodomo
Copy link
Contributor Author

lolodomo commented May 4, 2017

@resetnow : thank you for your quick review, I fixed what you requested.

Copy link
Contributor

@vlad-ivanov-name vlad-ivanov-name left a comment

Choose a reason for hiding this comment

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

Please remove a stray space, otherwise LGTM.

Thanks!

_t.value.innerHTML = value;
}

if (_t.count === 1) {
return;
}

if (_t.suppressUpdateButtons ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(space here)

Fix #3143
Fix #3346
Fix #3349

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

lolodomo commented May 4, 2017

Space suppressed.

@sjsf sjsf merged commit 611ede7 into eclipse-archived:master May 5, 2017
@sjsf
Copy link
Contributor

sjsf commented May 5, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants