Move the search widget into the consolidated charmbrowser view #353
Conversation
…in charmbrowser view
|
Test FAILed. |
|
Test PASSed. |
…ed other failings.
| searchWidget.EVT_SEARCH_GOHOME, this._goHome, this) | ||
| ); | ||
|
|
||
| this.after('withHomeChange', function(e) { |
There was a problem hiding this comment.
Is there a reason this event doesn't use addEvent?
There was a problem hiding this comment.
It's attached to the main instance, so when it goes away so does the event handler.
|
Test FAILed. |
|
I had a couple of QA issues. One I couldn't reproduce reliably, but I suspect it is related to the main issue. I suspect it may be an issue that was already existing, not introduced in this branch. The charmbrowser is being rendered more than once. To reproduce drag a service to the canvas, click on the service and open the inspector, close the inspector and then inspect the HTML of the sidebar and it will have two sets of charmbrowser HTML: |
|
It appears that the issue you're experiencing is also in develop. Can you elaborate on your other issue so I can see if it needs to be tackled in this branch? |
|
@hatched The other issue was that sometimes the home button rendering was a bit strange, but I'm almost certain this was because the other instance of the charmbrowser was showing underneath. I couldn't figure out how to reproduce it reliably. |
|
👍 QA OK. The double render probably needs another branch. |
|
Thanks, I'll look into it. Because that one condition is pre-existing I may push it to a follow-up |
| if (e.change) { | ||
| change = Y.merge(change, e.change); | ||
| } | ||
| if (window.flags && window.flags.il) { |
There was a problem hiding this comment.
Necessary since this branch requires il?
There was a problem hiding this comment.
Artifact of the copy/paste from the original code. I've left them all in so that they can all be removed at once.
|
Review looks good, with the possible caveat about the |
|
I'm seeing a rendering issue with the borders between items in the autocomplete dropdown; maybe some CSS that's missing? |
|
Thanks for the review and QA! The next branch after this one will be removing the There is definitely a css issue in the results list, I'll create a follow-up card for that. |
|
👍 for landing this one. QA OK. |
|
Thanks for the reviews and qa's follow-up cards have been created. |
|
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge |
|
Test passed when run manually: http://ci.jujugui.org:8080/job/juju-gui/1131/ |
This branch moves the search widget into the consolidated charmbrowser view. #### Note After this branch lands you will be REQUIRED to use the `il` flag until it's completely removed as some changes in this branch were too much work to implement just to keep the flag working for a few more days. #### To QA Use the `il` flags This branch moves the search widget so do a deep exploratory QA with the search widget and charmbrowser code.
This branch moves the search widget into the consolidated charmbrowser view.
Note
After this branch lands you will be REQUIRED to use the
ilflag until it's completely removed as some changes in this branch were too much work to implement just to keep the flag working for a few more days.To QA
Use the
ilflagsThis branch moves the search widget so do a deep exploratory QA with the search widget and charmbrowser code.