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

Toolbar Simplification #6290

Merged
merged 2 commits into from
Oct 12, 2019
Merged

Toolbar Simplification #6290

merged 2 commits into from
Oct 12, 2019

Conversation

epwinchell
Copy link
Contributor

@epwinchell epwinchell commented Oct 9, 2019

This PR removes all top level icons when text is also present, with the exception of custom button groups.

Old
Screen Shot 2019-10-09 at 12 44 38 PM

New
Screen Shot 2019-10-09 at 12 43 15 PM

follow-up to: #5997
Issue: #5937

@epwinchell
Copy link
Contributor Author

@miq-bot add_label formatting/styling, ivanchuk/no

@epwinchell
Copy link
Contributor Author

@miq-bot add_reviewer @skateman

@epwinchell epwinchell changed the title [WIP] Toolbar Simplification Toolbar Simplification Oct 9, 2019
@miq-bot miq-bot requested a review from skateman October 9, 2019 16:40
@epwinchell
Copy link
Contributor Author

@miq-bot assign @martinpovolny

@@ -2,7 +2,7 @@ class ApplicationHelper::Toolbar::AnsibleCredentialCenter < ApplicationHelper::T
button_group('ansible_repository', [
select(
:ansible_credential_configuration,
'fa fa-cog fa-lg',
'',
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather go with nil instead of allocating an empty string each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skateman Updated

@epwinchell epwinchell requested a review from skateman October 11, 2019 22:58
@miq-bot
Copy link
Member

miq-bot commented Oct 11, 2019

Checked commits https://github.com/epwinchell/manageiq-ui-classic/compare/28ae9ba3a50c24802eb70bab6a2243fb9d7a3119~...36ef4978a7e413666db87af72f0dd143a8b61c65 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
221 files checked, 0 offenses detected
Everything looks fine. ⭐

@martinpovolny martinpovolny merged commit 10ab66b into ManageIQ:master Oct 12, 2019
@martinpovolny martinpovolny added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 12, 2019
@himdel
Copy link
Contributor

himdel commented Oct 14, 2019

@epwinchell Didn't quite catch all of them it seems ;)

provider

(I disagree with this in #5937 (comment), but if we're going that way, I guess we should remove all of them?)

@himdel
Copy link
Contributor

himdel commented Oct 14, 2019

This should help finding the rest (not claiming all of those are relevant, this is only a list of all icons mentioned at the right indent level after a select():

ems_container_center.rb
115:    select(
117:      'fa fa-lock fa-lg',

cloud_tenant_center.rb
23:    select(
40:      'fa fa-tachometer fa-1xplus',
49:      'fa fa-th-list',

ems_cloud_center.rb
89:    select(
91:      'fa fa-lock fa-lg',
108:      'fa fa-tachometer fa-1xplus',
117:      'fa fa-th-list',

ems_physical_infras_center.rb
96:    select(
98:      'fa fa-lock fa-lg',

ems_containers_center.rb
102:    select(
104:      'fa fa-lock fa-lg',

gtl_view.rb
29:    select(
31:      'fa fa-download fa-lg',

miq_capacity_view.rb
3:    select(
5:      'fa fa-download fa-lg',

template_clouds_center.rb
92:    select(
94:      'pficon pficon-add-circle-o fa-lg',

ems_physical_infra_center.rb
81:    select(
83:      'pficon pficon-screen fa-lg',
101:    select(
103:      'fa fa-lock fa-lg',

ems_clouds_center.rb
90:    select(
92:      'fa fa-lock fa-lg',

diagnostics_zone_center.rb
68:    select(
70:      'fa fa-filter fa-lg',

compare_view.rb
19:    select(
21:      'fa fa-download fa-lg',

ems_infras_center.rb
98:    select(
100:      'fa fa-lock fa-lg',

host_center.rb
130:    select(
132:      'fa fa-power-off fa-lg',

tasks_center.rb
12:    select(
14:      'pficon pficon-delete fa-lg',

diagnostics_server_center.rb
60:    select(
62:      'fa fa-filter fa-lg',

ems_infra_center.rb
102:    select(
104:      'fa fa-lock fa-lg',
119:    select(
121:      'pficon pficon-screen fa-lg',

drift_view.rb
19:    select(
21:      'fa fa-download fa-lg',

report_view.rb
37:    select(
39:      'fa fa-download fa-lg',

chargeback_center.rb
3:    select(
5:      'fa fa-download fa-lg',

x_vm_snapshot_center.rb
10:    select(
12:      'pficon pficon-delete fa-lg',

x_gtl_view.rb
29:    select(
31:      'fa fa-download fa-lg',

(courtesy of ag '^(( {4}(button|select))| {6}["'\''])' plus some filtering)

@skateman
Copy link
Member

@himdel the download button intentionally has an icon as it doesn't have text, the rest should go.

@himdel
Copy link
Contributor

himdel commented Oct 14, 2019

My last comment was under the assumption that we only want to remove icons from first-level dropdowns.
There will be more if this is supposed to also affect actual first-level buttons (like "Refresh").

@epwinchell
Copy link
Contributor Author

My last comment was under the assumption that we only want to remove icons from first-level dropdowns.

@himdel We're only dealing with first-level dropdowns for this issue.

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.

5 participants