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

BZ#145849913 Allows for image assets to be dynamically skinned #790

Merged
merged 5 commits into from
May 26, 2017

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented May 24, 2017

https://bugzilla.redhat.com/show_bug.cgi?id=1452396
https://www.pivotaltracker.com/story/show/145849913

TL;DR

  • (in production) all skinned image now change when the source file in /images change
  • bundling of assets is still possible by defining it by relative path
  • the skinning process hasn't changed at all. still need to link to client/app/skin (for style sheets and the like).
  • again, nothing changed with link, this pr essentially configures webpack so that we dynamically set asset urls in sass based on deployment environment, so when we're in production, we'll use the appropriate /images folder assets vs development
  • the process for adding a new image that supports skinning is to add build the url in the .sass file as follows: 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 example
  • webpack output hasn't changed, though there is some work to be done there... (duplicate folders etc)

I 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

screenshot from 2017-05-24 16-07-11
screenshot from 2017-05-24 16-08-46

Screenshots (before/after) showing support for replacing contents of /ui/service/images on the fly

but before you look at these I need you to do a few things

  1. look at the url, we're on a production url (NOT dev)
  2. in the after shots, ignore the blue in the nav header, all this pr touches is how the images are loaded not the css
  3. ok now your ready to now 👀's

before skinning

screenshot from 2017-05-24 17-20-41
screenshot from 2017-05-24 17-20-58

after skinning

screenshot from 2017-05-24 17-00-22
screenshot from 2017-05-24 17-00-05
screenshot from 2017-05-24 17-00-37

@AllenBW AllenBW added the wip label May 24, 2017
@AllenBW AllenBW force-pushed the BZ/#145849913-live-skinning branch from b35505e to c9e036c Compare May 24, 2017 16:04
@chriskacerguis
Copy link
Contributor

chriskacerguis commented May 24, 2017

@AllenBW do we still need the skinning dir? Also, should we have some docs on how to do a skin now?

Looking much 🌮

@chriskacerguis chriskacerguis self-assigned this May 24, 2017
@AllenBW
Copy link
Member Author

AllenBW commented May 24, 2017

@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

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
@AllenBW AllenBW force-pushed the BZ/#145849913-live-skinning branch from 5edde0b to 4f2edeb Compare May 24, 2017 20:57
@miq-bot
Copy link
Member

miq-bot commented May 24, 2017

Checked commits AllenBW/manageiq-ui-service@9e9776e~...4f2edeb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@serenamarie125
Copy link

@AllenBW has the rebranding always changed the selection color ? 👀

@AllenBW
Copy link
Member Author

AllenBW commented May 24, 2017

@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

@AllenBW AllenBW removed the wip label May 24, 2017
@AllenBW AllenBW changed the title [WIP]BZ#145849913 live skinning BZ#145849913 Allows for image assets to be dynamically skinned May 24, 2017
@serenamarie125
Copy link

ok @AllenBW i thought it may be helpful to ask questions now rather than later. I'll hold off

@AllenBW
Copy link
Member Author

AllenBW commented May 24, 2017

@serenamarie125 we're good to go now, outta wip, feel free to ask away 🌮

Copy link
Contributor

@chriskacerguis chriskacerguis left a 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!

@serenamarie125
Copy link

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

@AllenBW
Copy link
Member Author

AllenBW commented May 25, 2017

@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

@serenamarie125
Copy link

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 ?

@AllenBW
Copy link
Member Author

AllenBW commented May 25, 2017

@serenamarie125 If there is another issue this pr brought to light, it should be dealt with in another pr/pt.
The background color is not changed by this pr. Meaning there might very well be another color for production, (there actually is, its black ish) but it wasn't touched. Didn't change. And as such wasn't skinned for the example screen shots.

@serenamarie125
Copy link

Understood @AllenBW - I'm not requesting changes, just pointing out another issue.

@AllenBW
Copy link
Member Author

AllenBW commented May 25, 2017

@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.

@AllenBW
Copy link
Member Author

AllenBW commented May 25, 2017

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.

@serenamarie125
Copy link

great thanks @AllenBW

@chriskacerguis
Copy link
Contributor

@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? 😄

@serenamarie125
Copy link

Course sorry I read no UX approval needed 🤣

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chriskacerguis chriskacerguis added this to the Sprint 62 Ending Jun 5, 2017 milestone May 26, 2017
@chriskacerguis chriskacerguis merged commit 23c1ebb into ManageIQ:master May 26, 2017
@AllenBW AllenBW deleted the BZ/#145849913-live-skinning branch May 31, 2017 12:15
simaishi pushed a commit that referenced this pull request Jun 12, 2017
BZ#145849913 Allows for image assets to be dynamically skinned
(cherry picked from commit 23c1ebb)

https://bugzilla.redhat.com/show_bug.cgi?id=1460755
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit f1510302c1def92108c581ab771c79489fafa1a9
Author: Chris Kacerguis <[email protected]>
Date:   Fri May 26 08:34:31 2017 -0500

    Merge pull request #790 from AllenBW/BZ/#145849913-live-skinning
    
    BZ#145849913 Allows for image assets to be dynamically skinned
    (cherry picked from commit 23c1ebbe6e9b1de5bca052702a2d5e5ca022132b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460755

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