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

Load ui-components for printing with asset pipeline require #4941

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

skateman
Copy link
Member

This is a production-only issue, the SASS @import doesn't have the yarn packages in its load path. By calling the require this is solved. To reproduce it, click on the printer icon on any VM's summary screen.

Before:
screenshot from 2018-11-19 18-16-37

After:
screenshot from 2018-11-19 18-12-16

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1651194

@miq-bot add_reviewer @himdel
@miq-bot add_label bug, hammer/yes

@himdel
Copy link
Contributor

himdel commented Nov 19, 2018

LGTM,
for some reason, rails sass treats the import as an url import.

@import '~@manageiq/ui-components/dist/css/ui-components.css'; would only work with webpack (and that would probably break patternfly style overrides).

So, asset pipeline for the rescue 👍

Tested in UI, does not break in devel mode.
(Not tested on an appliance as it's currently impossible to build assets without running out of free space.)

Merging when green.

@miq-bot
Copy link
Member

miq-bot commented Nov 19, 2018

Checked commit skateman@3505e68 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@himdel himdel added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 19, 2018
@himdel himdel merged commit 9ad557b into ManageIQ:master Nov 19, 2018
@skateman skateman deleted the print-ui-components branch November 19, 2018 18:22
simaishi pushed a commit that referenced this pull request Nov 19, 2018
Load ui-components for printing with asset pipeline require

(cherry picked from commit 9ad557b)

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

Hammer backport details:

$ git log -1
commit 91dc4d59ed3fb7117f36719bbe9c388d4bf27bbb
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Nov 19 19:21:09 2018 +0100

    Merge pull request #4941 from skateman/print-ui-components
    
    Load ui-components for printing with asset pipeline require
    
    (cherry picked from commit 9ad557b14dd91cd151c51308429303aebd092849)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1651194

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