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

[property view] implement a new input for custom icons#120

Closed
zeroooing wants to merge 1 commit into
intel:masterfrom
zeroooing:property
Closed

[property view] implement a new input for custom icons#120
zeroooing wants to merge 1 commit into
intel:masterfrom
zeroooing:property

Conversation

@zeroooing

Copy link
Copy Markdown
Contributor

user can set a costom icon name, with suggestions
from a dropdown list greped by user's input

Comment thread src/js/views/property.js Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

change event will refresh property view so that click event of datalist is not triggered..

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please put the above comment in the CODE, such like:
FIXME: The "change" event will refresh property view so "click" event of datalist is not triggered. We have to look up the ":hover" class here to decide which item is clicked

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed as john's comments

user can set a costom icon name, with suggestions
from a dropdown list greped by user's input
Comment thread src/js/views/property.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a whitespace between "//" and FIXME :)

@grgustaf

Copy link
Copy Markdown
Contributor

There were some tight loops making DOM changes; it's more efficient to batch them up into a single DOM change whenever possible. I added a follow-on patch to do that, please take a look and make sure it looks right.

Also, with the old code the list of icon names appeared above the control, floating above the outline view. I believe it would go below if there was room, but it notices there isn't and goes above. It would be great if you could do that here too, because it's hard to use your control when it's partially scrolled off the screen.

But nice job...

Please close this PR, I'm leaving it open to try to make sure you get my comments.

@zeroooing zeroooing closed this Jul 16, 2012
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