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

New event handler #296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New event handler #296

wants to merge 1 commit into from

Conversation

xuqingkuang
Copy link
Contributor

Refined the the event handler to be a standalone View, and added a icon to indicate existence of event handlers in outline View.

@zhizhangchen
Copy link
Contributor

Please merge the two commits into one as the first commit can't exist by itself

@zhizhangchen
Copy link
Contributor

There's a problem: When creating a new event handler, the editor doesn't have a cursor

@zhizhangchen
Copy link
Contributor

Then icon is a little big and I think two branches lightning is better than three branches lightning.

$('.eventHandler').each( function() {
$(this).eventHandlerView();
$(this).eventHandlerView('option', 'model', ADM)
.dialog({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you create an dialog here instead of doing it in _create method of eventHandlerView?

@xuqingkuang
Copy link
Contributor Author

@zhizhangchen The code updated by your comments, please review.

There's only a issue is CodeMirror editor contents can't erase in some situation, so the editor re-initial code is exist in 'dialogopen' event handler and the 'refresh' function, it could make sure the editor is blank after selectionChanged.

Refined the the event handler to be a standalone View,
and added a icon to indicate existence of event handlers in outline View.
@@ -1563,17 +1563,19 @@ input.screenCoordinate::-webkit-inner-spin-button {
margin: 0 0.6em;
}

#eventHandlerDialog {
.eventHandler {
width: 980px !important; /* Override computed style */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is generally a bad idea, hardcoding sizes. If my screen or window is small enough, it's impossible to access the bottom of the dialog as it does not scroll it's contents.

I'd prefer we set max, min widths and heights in hard pixels only if necessary, then use percentages for the default width and heights. Also make sure the contents of the dialog can be scrolled if the dialog it's self is too small.

@sbryan
Copy link
Contributor

sbryan commented Sep 14, 2012

The eventHandlerIcon.png does not really seem to fit or match the existing color scheme of the UX. I won't hold the PR becuase of this, but I suggest if we are going to try and be creative, we should follow existing color schemes. Normally, we would request new visual assets from our creative team, but, since we don't have one... ;)

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

Successfully merging this pull request may close these issues.

3 participants