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

Charmbrowser dispatcher #235

Merged
merged 2 commits into from Apr 11, 2014
Merged

Conversation

hatched
Copy link
Contributor

@hatched hatched commented Apr 11, 2014

This branch now uses the new state object to render the charmbrowser urls.

Notes

This branch is blatantly missing tests for this new functionality.

To QA

Visit the following urls:

  • /precise/apache2/:flags:/state/?text=apache2#related-charms ( should show the search results, charm details, and related-charms pane )
  • /precise/apache2/:flags:/state/#related-charms ( should show the editorial results, charm details and the related-charms pane )
  • /precise/apache2/:flags:/state/ ( should show the charm details )
  • /:flags:/state/?text=apache2 ( should show the search results )

@jujugui
Copy link
Contributor

jujugui commented Apr 11, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/713/

@@ -388,7 +398,26 @@ YUI.add('subapp-browser', function(Y) {
@param {Object|String} metadata The metadata to pass to the charmbrowser
view.
*/
_charmbrowser: function(metadata) {},
_charmbrowser: function(metadata) {
// If there is no provided metadata show the defaults.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this comment refers to code at line 408 and should be move below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, moving, thanks

@frankban
Copy link
Member

👍 and QA ok.
Thank you Jeff!

@hatched
Copy link
Contributor Author

hatched commented Apr 11, 2014

Thanks for the review! Good catches, all fixed!

@hatched
Copy link
Contributor Author

hatched commented Apr 11, 2014

:shipit:

@jujugui
Copy link
Contributor

jujugui commented Apr 11, 2014

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge

@jujugui
Copy link
Contributor

jujugui commented Apr 11, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/714/

jujugui added a commit that referenced this pull request Apr 11, 2014
This branch now uses the new state object to render the charmbrowser urls.

#### Notes
This branch is blatantly missing tests for this new functionality.

#### To QA
Visit the following urls:
- /precise/apache2/:flags:/state/?text=apache2#related-charms ( should show the search results, charm details, and related-charms pane )
- /precise/apache2/:flags:/state/#related-charms ( should show the editorial results, charm details and the related-charms pane )
- /precise/apache2/:flags:/state/ ( should show the charm details )
- /:flags:/state/?text=apache2 ( should show the search results )
@jujugui jujugui merged commit 7ddabb9 into juju:develop Apr 11, 2014
@hatched hatched deleted the charmbrowser-dispatcher branch April 11, 2014 20:09
@mitechie
Copy link
Contributor

Can you explain why there's blatantly no tests?

@@ -431,10 +460,17 @@ YUI.add('subapp-browser', function(Y) {
@param {function} next callable for the next route in the chain.
*/
renderEntityDetails: function(req, res, next) {
var entityId = this.state.getCurrent('charmID');
var entityId, hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

newline vars

@hatched
Copy link
Contributor Author

hatched commented Apr 12, 2014

@mitechie missing tests? The only things that are missing tests in this commit are things which will be refactored out like the sidebar and routeDefault methods.

@jujugui
Copy link
Contributor

jujugui commented Apr 12, 2014

Sorry I was replying to the pull request comment you made of " This branch
is blatantly missing tests for this new functionality"
On Apr 12, 2014 10:26 AM, "Jeff Pihach" notifications@github.com wrote:

@mitechie https://github.com/mitechie missing tests? The only things
that are missing tests in this commit are things which will be refactored
out like the sidebar and routeDefault methods.

Reply to this email directly or view it on GitHubhttps://github.com//pull/235#issuecomment-40281657
.

@hatched
Copy link
Contributor Author

hatched commented Apr 12, 2014

Oh, right, that was in reference to the stuff that was going to be refactored out :-)

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

Successfully merging this pull request may close these issues.

None yet

4 participants