-
Notifications
You must be signed in to change notification settings - Fork 91
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
BZ#145849913 Allows for image assets to be dynamically skinned #790
BZ#145849913 Allows for image assets to be dynamically skinned #790
Conversation
b35505e
to
c9e036c
Compare
@AllenBW do we still need the skinning dir? Also, should we have some docs on how to do a skin now? Looking much 🌮 |
@chriskacerguis this doesn't change anything with skinning, other than now it works, still gotta do the whole link thing, will explain all in the pr before removing the wip |
Fix icon list image
Both patternfly.scss and shopping-cart.scss need to be sass for webpacks sass-loader variable injection to be happy
When an url() value might need to be skinned, append to the path $imgBasePath var. For production this will (usually) be `/ui/service` for dev this will always be `/` see _application.sass for example usage
In order to inject the data variable $imgBasePath into our sass files, we have to breakout processing them from the css (otherwise webpack complains) $imgBasePath defaults to / when we're not in production and /ui/service/ when we are
e17ea46
to
5edde0b
Compare
5edde0b
to
4f2edeb
Compare
Checked commits AllenBW/manageiq-ui-service@9e9776e~...4f2edeb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@AllenBW has the rebranding always changed the selection color ? 👀 |
@serenamarie125 this is still a wip, working on getting better screen shots, see the comment i made before the odd color ss, also mentioned there, i only replaced the images |
ok @AllenBW i thought it may be helpful to ask questions now rather than later. I'll hold off |
@serenamarie125 we're good to go now, outta wip, feel free to ask away 🌮 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice elegant fix. Much wow!
I'm not sure if I'm providing information in the correct area, but the colors you are showing for the ManageIQ login screen here are incorrect |
@serenamarie125 again, as I mentioned in the pr, the only thing that has been skinned are the images, the colors have unmodified (this pr doesn't touch them, so I didn't touch them in the ss) no ux has changed here |
Understood @AllenBW, but there is still an issue, so I'm calling it out. @chriskacerguis can you follow up to make sure that the background color of the CloudForms Login screen is the correct color ? |
@serenamarie125 If there is another issue this pr brought to light, it should be dealt with in another pr/pt. |
Understood @AllenBW - I'm not requesting changes, just pointing out another issue. |
@serenamarie125 gotcha, so I'm saying it's not another issue. Last time I checked the background colors changed for production, the images did not. This doesn't touch the former, only latter. As has been previously stated, the colors might be wrong, they were not changed in the ss, only the images were. |
In fact, if you follow this pr to the bz, you'll see the background colors are correct, again this pr didn't change that, they'll still be correct. |
great thanks @AllenBW |
@serenamarie125 going over the comments I wasn't 100% sure if you were good with this. If you are could you please add your approval? 😄 |
Course sorry I read no UX approval needed 🤣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
BZ#145849913 Allows for image assets to be dynamically skinned (cherry picked from commit 23c1ebb) https://bugzilla.redhat.com/show_bug.cgi?id=1460755
Fine backport details:
|
https://bugzilla.redhat.com/show_bug.cgi?id=1452396
https://www.pivotaltracker.com/story/show/145849913
TL;DR
/images
changeclient/app/skin
(for style sheets and the like).url('#{$img-base-path}images/foo.(jpg|png|etc)')
where foo is the file to be skinned and the extension is whatever checkout _navigation.sass for an exampleI can proudly say, no ux review is necessary as nothing visually changed 🤘 🌮 💃 (except those things that are supposed to change, ie skinned images).
Production build file structure before, the second one is after, just showing nothing changed
Screenshots (before/after) showing support for replacing contents of
/ui/service/images
on the flybut before you look at these I need you to do a few things
before skinning
after skinning