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

Fix broken stylesheet path for PDFs #14793

Merged
merged 1 commit into from
May 12, 2017
Merged

Conversation

hayesr
Copy link
Contributor

@hayesr hayesr commented Apr 18, 2017

The code points to the wrong place after the UI repo split. This changes it to point to the stylesheet under the UI engine.

This fixes basic PDF styles, but images remain to be addressed.

Dependent on ManageIQ/manageiq-ui-classic#1332 to work.

Related:

Currently
screen shot 2017-05-11 at 11 29 29 am

After
screen shot 2017-05-11 at 11 32 58 am

@hayesr
Copy link
Contributor Author

hayesr commented Apr 18, 2017

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Apr 18, 2017
@imtayadeway
Copy link
Contributor

@hayesr could this be related to https://bugzilla.redhat.com/show_bug.cgi?id=1442067 ?

@hayesr
Copy link
Contributor Author

hayesr commented Apr 18, 2017

@imtayadeway It could indeed. Not sure how graphs are generated, this fixes basic stylesheets, but things like images will take more. (I'm also looking into that)

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

I think this logic neeeds to be more pluggable to find stylesheets provided by other pluggable components.

@hayesr
Copy link
Contributor Author

hayesr commented Apr 19, 2017

@bdunne perhaps this generator shouldn't try to figure out the path at all. We can just expect the thing that's calling it to use the right path.

@hayesr
Copy link
Contributor Author

hayesr commented Apr 20, 2017

Is there an easy fix to make this pluggable? Otherwise, this fixes a bug now and maybe we can punt making it flexible until that's needed?

@bdunne
Copy link
Member

bdunne commented Apr 25, 2017

@hayesr something like this would work:

Vmdb::Plugins.instance.vmdb_plugins.each do |plugin|
  plugin.root.join("app/assets/stylesheets", whatever)
end

You may want to use detect rather than each if you're only looking for one. And you'll need to see is the file exists.

@hayesr
Copy link
Contributor Author

hayesr commented May 9, 2017

@bdunne I run into a weird situation where Vmdb::Plugins.instance.vmdb_plugins is empty some times, any ideas?

@bdunne
Copy link
Member

bdunne commented May 9, 2017

It can be an empty array if you reload! in dev mode.

@hayesr hayesr force-pushed the fix_pdf_css_path branch from f5eb0ec to 37ca9df Compare May 9, 2017 23:09
@hayesr hayesr force-pushed the fix_pdf_css_path branch from 37ca9df to 46719a3 Compare May 11, 2017 18:15
@miq-bot
Copy link
Member

miq-bot commented May 11, 2017

Checked commit hayesr@46719a3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@dclarizio
Copy link

@hayesr can you add a before/after screenshot of the PDFs? Thx, Dan

@hayesr
Copy link
Contributor Author

hayesr commented May 11, 2017

Restarting CI

@hayesr hayesr closed this May 11, 2017
@hayesr hayesr reopened this May 11, 2017
hayesr added a commit to hayesr/manageiq-ui-classic that referenced this pull request May 11, 2017
@hayesr
Copy link
Contributor Author

hayesr commented May 11, 2017

@bdunne Ready for another look.

@h-kataria
Copy link
Contributor

looks good

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@bdunne bdunne merged commit 49005d9 into ManageIQ:master May 12, 2017
@bdunne bdunne added this to the Sprint 61 Ending May 22, 2017 milestone May 12, 2017
@bdunne bdunne added the euwe/no label May 12, 2017
@simaishi
Copy link
Contributor

@hayesr @bdunne can this be euwe/yes? The BZ has the flag set and ManageIQ/manageiq-ui-classic#1332 is euwe/yes as well.

@bdunne
Copy link
Member

bdunne commented May 15, 2017

@simaishi No, Vmdb::Plugins doesn't exist in euwe, and the UI wasn't split at that time. I think we need a separate PR for euwe.

@simaishi
Copy link
Contributor

@bdunne thank you.
@hayesr I'll mark as euwe/conflict for now, please remove the label when Euwe PR is ready.

@hayesr
Copy link
Contributor Author

hayesr commented May 23, 2017

@simaishi If #15131 can be backported it would solve the same problem.

@simaishi
Copy link
Contributor

@hayesr In that case, is ManageIQ/manageiq-ui-classic#1332 still needed or just need ManageIQ/manageiq-ui-classic#1363?

With multiple PRs it's getting harder to track what's needed and not. Can you please add/remove fine/yes and euwe/yes labels where needed? I won't backport any until all needed PRs for the BZ are merged, so the labels can be added later if that's easier.

@hayesr
Copy link
Contributor Author

hayesr commented May 23, 2017

@miq-bot remove_label euwe/conflict euwe/yes
@miq-bot add_label euwe/no fine/no

@simaishi Ok, changing this one to euwe/no fine/no because the other solution is better. (branch-specific versions of #15131) may still be needed, though) Both UI PRs are needed.

@miq-bot
Copy link
Member

miq-bot commented May 23, 2017

@hayesr Cannot remove the following label because they are not recognized: euwe/conflict euwe/yes

@miq-bot
Copy link
Member

miq-bot commented May 23, 2017

@hayesr Cannot apply the following label because they are not recognized: euwe/no fine/no

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.

9 participants