Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/app switcher #4994

Merged
merged 16 commits into from
Sep 23, 2015
Merged

Fix/app switcher #4994

merged 16 commits into from
Sep 23, 2015

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Sep 21, 2015

Fixes #4990

This adds an appSwitcher directive that currently just reacts to the clicks on apps and ensures that they cause a full page load.

@spalger spalger added the review label Sep 21, 2015
@Bargs
Copy link
Contributor

Bargs commented Sep 22, 2015

LGTM

@Bargs Bargs assigned jbudz and unassigned Bargs Sep 22, 2015
jbudz
jbudz reviewed Sep 22, 2015
View reviewed changes
src/ui/public/chrome/appSwitcher/appSwitcher.js
controller: function () {

// links don't cause full-navigation events in certain scenarios
// so we force them when needed
Copy link
Member

Choose a reason for hiding this comment

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

Could use a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@jbudz jbudz assigned spalger and unassigned jbudz Sep 22, 2015
@w33ble
Copy link
Contributor

w33ble commented Sep 22, 2015

What's up with all the URL changes when you click on the active app. Is that just a weird side effect or is it an issue? Adding stopPropagation fixes this, and it sounds like that change is coming... Once that change is made, LGTM.

2015-09-22 14_00_41

Also note here that I modified the icon padding - @app-icon-padding: 20px;, I think it looks really nice with added whitespace. Thoughts?

@spalger spalger assigned Bargs and unassigned spalger Sep 22, 2015
@rashidkpc rashidkpc assigned jbudz and unassigned Bargs Sep 23, 2015
jbudz
jbudz reviewed Sep 23, 2015
View reviewed changes
src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js
require('ui/chrome/appSwitcher');
var DomLocationProvider = require('ui/domLocation');

function findTestSubject() {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be duplicated with findTestSubject.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't mean to include findTestSubject.js. Planned to replace with #5001

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed findTestSubject.js

@jbudz
Copy link
Member

jbudz commented Sep 23, 2015

Clickin on(reloading) the current app is dropping the href
edit: nevermind, working fine

@jbudz
Copy link
Member

jbudz commented Sep 23, 2015

LGTM, sending to @lukasolson for seconds

@jbudz jbudz assigned lukasolson and unassigned jbudz Sep 23, 2015
@lukasolson
Copy link
Member

When you click on the apps tab, then click on one of the other tabs, the apps tab remains open. Is this by design, or a bug?

2015-09-23 10_32_46

Sorry, I should have captured down below. The actual page is changing even though you can't see it in the screen capture.

@spalger
Copy link
Contributor Author

spalger commented Sep 23, 2015

Hmm, it's probably not intentional but does seem strange. Does the timepicker have the same behavior?

@lukasolson
Copy link
Member

Yeah, it does. I think it's probably fine.

@lukasolson lukasolson closed this Sep 23, 2015
@lukasolson lukasolson reopened this Sep 23, 2015
@spalger
Copy link
Contributor Author

spalger commented Sep 23, 2015

Cool

lukasolson
lukasolson reviewed Sep 23, 2015
View reviewed changes
src/ui/public/domLocation.js
@@ -0,0 +1,15 @@
module.exports = function DomLocationProvider($window) {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why do you create this module instead of using something like $location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$location doesn't provide the whole url, just the url as the angular router sees it (the part after the hash). Since we have different apps with potentially different base urls (like status page) I needed the whole url

Copy link
Member

Choose a reason for hiding this comment

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

Ah, $location.absUrl is only a getter, not a setter. Makes sense. Thanks

@lukasolson
Copy link
Member

LGTM.

lukasolson added a commit that referenced this pull request Sep 23, 2015
@lukasolson
Merge pull request #4994 from spalger/fix/appSwitcher
0b06d1a 
Fix/app switcher
@lukasolson lukasolson merged commit 0b06d1a into elastic:master Sep 23, 2015
@spalger spalger deleted the fix/appSwitcher branch February 25, 2016 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lukasolson lukasolson

Labels
review v4.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spalger @Bargs @w33ble @jbudz @lukasolson @rashidkpc