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

Add missing icons for provider policies & compliance events #94

Merged
merged 2 commits into from
Jan 19, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Jan 8, 2017

Followup to ManageIQ/manageiq#11002,
https://bugzilla.redhat.com/show_bug.cgi?id=1411337 (master)
https://bugzilla.redhat.com/show_bug.cgi?id=1411364 (euwe)

Before:

ems-event-missing-icons
screenshot-assigned-to-policy-ems
screenshot-provider-policy-noicons

After (this PR only fixes icons, event name fixed in core PR ManageIQ/manageiq#13388):

screenshot-provider-events
screenshot-event-fixed
screenshot-provider-policy
screenshot-provider-policy-profiles-disabled

miq_policy_extmanagementsystem_inactive.png made with red "X" imperfectly stolen from other _inactive icons.

@cben
Copy link
Contributor Author

cben commented Jan 8, 2017

@epwinchell Please review

@skateman
Copy link
Member

skateman commented Jan 12, 2017

@cben I think this is a regression that will be fixed by merging #128, could you please verify it? We want to get rid of the PNG files and use fonticons where possible.

@epwinchell
Copy link
Contributor

epwinchell commented Jan 12, 2017

@skateman
Copy link
Member

@epwinchell this should do it:

%i{:class => @event.decorate.fonticon}

@cben
Copy link
Contributor Author

cben commented Jan 12, 2017

Cool, checking...

@miq-bot add-label euwe/yes
(euwe BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1411364)
#128 is euwe/no, so perhaps this is still needed.

@epwinchell
Copy link
Contributor

@cben PR looks fine

@epwinchell
Copy link
Contributor

@miq-bot add_label bug

@epwinchell
Copy link
Contributor

@miq-bot assign @himdel

@cben
Copy link
Contributor Author

cben commented Jan 12, 2017

Hmm, this won't merge as-is to uewe, miq_event_definition_decorator.rb was much smaller then.

However. no, #128 alone does not fix any of those icons.
It does make event-*png unnecessary, given the miq_event_definition_decorator.rb additions from this PR.
miq_policy_*png are still needed, git grep 100/miq_policy_ still returns 8 places it's used.

Oops I forgot miq_policy_extmanagementsystem_inactive.png. FIXED
@miq-bot add-label wip

@miq-bot miq-bot added the wip label Jan 12, 2017
@cben cben changed the title Add missing icons for Provider Compliance * events. [WIP] Add missing icons for Provider Compliance * events. Jan 12, 2017
@epwinchell
Copy link
Contributor

@cben Doing a separate PR for Euwe with just the images should take care of it.

@cben cben changed the title [WIP] Add missing icons for Provider Compliance * events. Add missing icons for provider policies & compliance events Jan 16, 2017
@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2017

Checked commits cben/manageiq-ui-classic@c4bc393~...fbf0c84 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 🍰

@cben
Copy link
Contributor Author

cben commented Jan 16, 2017

Added inactive policy icon (see last screenshot).
@miq-bot remove-label wip

Kept event-*.png files in this PR, so this may be merged before #128. The other event-*.png files still exist, you'll later be able to delete 176 instead of 173 of them...

Created euwe PR ManageIQ/manageiq#13502.

@miq-bot miq-bot removed the wip label Jan 16, 2017
@cben
Copy link
Contributor Author

cben commented Jan 19, 2017

Ping, any chance to merge & backport before euwe-2 release?

@mzazrivec mzazrivec added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 19, 2017
@mzazrivec mzazrivec merged commit 92a56fa into ManageIQ:master Jan 19, 2017
@himdel himdel removed their assignment Jan 19, 2017
@simaishi
Copy link
Contributor

Backported to Euwe via ManageIQ/manageiq#13502

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.

7 participants