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.

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

Do not append title to breadcrumbs on show_list pages #5904

Merged
merged 2 commits into from Aug 19, 2019

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Jul 30, 2019

closes #5361

Description

When a header on the show_list page and a title in the menu are not the same, only the menu title is used.

Before

image

After

image

@miq-bot add_label changelog/yes, ivanchuk/no, breadcrumbs

@rvsia rvsia force-pushed the menu-hiearchy branch 2 times, most recently from afc2f69 to b856a23 Compare July 30, 2019 11:42
@rvsia rvsia changed the title [WIP] Do not append title to breadcrumbs on show_list pages Do not append title to breadcrumbs on show_list pages Jul 31, 2019
@rvsia
Copy link
Contributor Author

rvsia commented Jul 31, 2019

@himdel Can you please check if everything is OK in optimization controller? 🙏 Thanks!

@miq-bot miq-bot removed the wip label Jul 31, 2019
@@ -24,19 +24,12 @@ def breadcrumbs_options
{
:breadcrumbs => [
{:title => _('Overview')},
bc_optimization,
{:title => _("Optimization"), :url => url_for_only_path(:action => 'show_list', :id => nil)},
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@himdel himdel Aug 15, 2019

Choose a reason for hiding this comment

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

You wil need to undo this change, #6041 changes how optimization does breadcrumbs, this PR won't affect it anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends what will be merged sooner... 😏

I will undo it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, and if you can rebase, I'll try to test this one tomorrow :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I’ll do it tomorrow.

@himdel
Copy link
Contributor

himdel commented Jul 31, 2019

Optimization is fine :) (but that's one of the few places where every screen has a breadcrumbs test :))

@rvsia
Copy link
Contributor Author

rvsia commented Aug 15, 2019

@miq-bot remove_label ivanchuk/no
@miq-bot add_label ivanchuk/yes

@rvsia
Copy link
Contributor Author

rvsia commented Aug 16, 2019

@himdel So I removed all changes to optimization controller 🗡️

@himdel
Copy link
Contributor

himdel commented Aug 16, 2019

(test failures will go away after #6041)

@rvsia
Copy link
Contributor Author

rvsia commented Aug 16, 2019

@himdel so let's merged #6041 first and then this (and I will also fix the rubocop issues)

@himdel
Copy link
Contributor

himdel commented Aug 16, 2019

LGTM, will merge green 👍

The only issue I am seeing is in the usual place - /generic_object_definition/show_list shows Automation > Automate > Generic Objects > All Generic Object Classes > All Generic Object Classes instead of just Automation > Automate > Generic Objects

(and there is a tree even though it's a show_list, so, it kinda makes sense, for now, until we can make it into a proper explorer or something consistent with the other places)

@himdel himdel self-assigned this Aug 16, 2019
@miq-bot
Copy link
Member

miq-bot commented Aug 16, 2019

Checked commits rvsia/manageiq-ui-classic@28fe48a~...efd96c8 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
35 files checked, 0 offenses detected
Everything looks fine. 🏆

@himdel himdel closed this Aug 19, 2019
@himdel himdel reopened this Aug 19, 2019
@himdel himdel merged commit 727742e into ManageIQ:master Aug 19, 2019
@himdel himdel added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 19, 2019
@rvsia rvsia deleted the menu-hiearchy branch September 17, 2019 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary 4th level of hierarchy
4 participants