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

Unit details breakout pane is a dispatched view. #426

Merged
merged 1 commit into from Jul 9, 2014

Conversation

jcsackett
Copy link
Contributor

This changes the unit details breakout pane to a dispatched view, rather than simply a show/hideSlot.

@jcsackett
Copy link
Contributor Author

QA:

Open a unit's details; url should change to /unit/$unitnumber

Close the unit breakout pane; url should remove the /unit bit

Open again, then hit back button. Pane should close.

@jujugui
Copy link
Contributor

jujugui commented Jul 9, 2014

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

this.showViewlet('charmDetails', charm);
} else if (activeUnit >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're checking here if activeUnit is above 0 but it's default value is undefined and it can be assigned an undefined value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually checking if it's greater than or equal to 0; if it's undefined this will be falsey, but if i just do the if (activeUnit) when activeUnit is 0 it'll fail.

Would you prefer I do if typeof Number?

@hatched
Copy link
Contributor

hatched commented Jul 9, 2014

QA Issue:

With the unit details pane open click the charm details link, the url adds /charm changes but the unit url is still there and the unit pane is still visible.

@jujugui
Copy link
Contributor

jujugui commented Jul 9, 2014

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

@hatched
Copy link
Contributor

hatched commented Jul 9, 2014

Thanks for this, having these new urls is going to be really nice.
QA OK

As mentioned on IRC, it would be nice if you could add tests to make sure the proper state objects are being fired wrt the bug which you fixed.

👍 with those tests.

@jujugui
Copy link
Contributor

jujugui commented Jul 9, 2014

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

@jcsackett
Copy link
Contributor Author

I have added the tests. Let's :shipit:

@jujugui
Copy link
Contributor

jujugui commented Jul 9, 2014

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

jujugui added a commit that referenced this pull request Jul 9, 2014
Unit details breakout pane is a dispatched view.

This changes the unit details breakout pane to a dispatched view, rather than simply a show/hideSlot.
@jujugui jujugui merged commit fec0ab2 into juju:develop Jul 9, 2014
@jcsackett jcsackett deleted the dispatch-unit-details branch July 9, 2014 21:20
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

3 participants