Keep Reporting menu in Nav Menu when switching Index Patterns #299
Conversation
…ing menu disappearing
davidcui1225
requested review from
joshuali925,
akbhatta,
zhongnansu and
abbashus
Codecov Report
@@ Coverage Diff @@
## dev #299 +/- ##
=======================================
Coverage 77.67% 77.67%
=======================================
Files 32 32
Lines 1805 1805
Branches 356 356
=======================================
Hits 1402 1402
Misses 398 398
Partials 5 5 Continue to review full report at Codecov.
|
| @@ -234,12 +234,34 @@ $(function () { | |||
| locationHashChanged(); | |||
| }); | |||
|
|
|||
| const isDiscoverNavMenu = (navMenu) => { | |||
| return navMenu[0].children.length === 5 && | |||
There was a problem hiding this comment.
why is it always 5, 4, or 3. I feel it's kind of fragile to used == (a fixed number)
There was a problem hiding this comment.
It checks how many items are in the nav menu depending on what kind of report source it is- I can extract them to constants too
There was a problem hiding this comment.
can we remove this length check, since you have the title check？Or can we just use length >= 0 or length >= 1? What's the downside you see
There was a problem hiding this comment.
That was the old check, which resulted in the issue. Basically it would not add the Reporting menu if the nav menu had length greater than 1, which is why the Reporting menu would not show up. But we need to not only check that the Nav menu is populated, but that it has the correct number of items
There was a problem hiding this comment.
a better way might be to add an id to reports menu element, loop through navMenu and check for the id to determine whether reports menu is added. probably a todo item
There was a problem hiding this comment.
+1 - I think that's a good idea but it doesn't sound like a quick fix
| const isDiscoverNavMenu = (navMenu) => { | ||
| return navMenu[0].children.length === 5 && | ||
| ($('[data-test-subj="breadcrumb first"]').prop('title') === 'Discover' || | ||
| $('[data-test-subj="breadcrumb first last"]').prop('title') === 'Discover') |
There was a problem hiding this comment.
why there are 2 conditions being checked, while the other functions contain one.
There was a problem hiding this comment.
Because on the Discover page the Reporting menu will be present in the menu even if there is no Saved search selected, but the id of the Discover breadcrumb prop is different when there is a Saved search selected vs. when there isn't
There was a problem hiding this comment.
got it. So, this only happens with Discover, not the other two?
There was a problem hiding this comment.
Yeah- because on the base Dashboard and Visualizations screens it is just a list of the available ones, but you are not on the actual page with the nav menu
There was a problem hiding this comment.
Did you try the "create visualization/dashboard"? I think it leads to a similar scenario with Discover, with no saved object selected, but with a nav bar
There was a problem hiding this comment.
Good catch- I will have to add those checks as well
…ehavior from pre-change
Issue #, if available:
N/A
Description of changes:
DiscoverpageBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.