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

Minify media viewer only #2195

Merged
merged 16 commits into from
Apr 2, 2014
Merged

Minify media viewer only #2195

merged 16 commits into from
Apr 2, 2014

Conversation

atarkowska
Copy link
Member

This PR replaced #2161. Created on top of #2194.
Those changes allows to embed image viewer on the static HTML website. All static files are minified using django-pipline and served on:

http://HOST/static/omeroweb.viewer.min.css
http://HOST/static/omeroweb.viewer.min.js

To test it remotely please go to https://github.com/aleksandra-tarkowska/openmicroscopy/tree/minify_media_viewer_only/components/tools/OmeroWeb/omeroweb/webtest/templates/webtest/embeding and download embedded.html. Then replace http://localhost in the following:

<link rel="stylesheet" type="text/css" href="http://localhost/static/omeroweb.viewer.min.css">
<script type="text/javascript" src="http://localhost/static/omeroweb.viewer.min.js"></script>

with the appropriate remote server name including custom prefix like http://remote_host/omero/

If you wish to test this PR locally you can use static page in webtest/templates/webtest/embed. Double click on the file and see if you can view image in the viewer.

In order to minify you need to run bin/omero web start that runs collectstatic command, your CSS and your javascripts will be compressed in the same time. If you run your development server directly you need to run python manage.py collectstatic first.

This was referenced Mar 21, 2014
@jburel jburel added the develop label Mar 21, 2014
@will-moore
Copy link
Member

The static webtest embed.html works fine locally.
Just for the record, there was a console error coming from the minified Raphael library but this has no effect on the viewer functionality (ROIs not shown).
TypeError: (intermediate value)(...) is not a function

Don't know if this actually breaks any potential use of Raphael or not?

@atarkowska
Copy link
Member Author

Thanks @will-moore, that was a known issue. I would prefer to tackle Raphael library in the separate PR

@atarkowska
Copy link
Member Author

@cneves ,@knabar, @manics, @dpwrussell would you like to make any comments on that PR?

@atarkowska
Copy link
Member Author

@knabar looks like we do not need GZipMiddleware at all as it is used for HTML compression only, see https://github.com/cyberdelia/django-pipeline/blob/master/docs/usage.rst#middleware

@knabar
Copy link
Member

knabar commented Mar 25, 2014

@aleksandra-tarkowska Makes sense. That was the only thing that jumped out when I reviewed the code, I'll still run the changes on my local install to see if I can find anything else.

@atarkowska
Copy link
Member Author

Great thanks

@knabar
Copy link
Member

knabar commented Mar 25, 2014

Some basic tests revealed no issues with this PR, although I found some other problems that were introduced some time ago (see PR #2205).

@atarkowska
Copy link
Member Author

The latest develop has been merged inluding PR #2205

@atarkowska
Copy link
Member Author

@knabar could you check whether we are missing anything else?

@knabar
Copy link
Member

knabar commented Apr 1, 2014

@aleksandra-tarkowska Everything else looks good

@chris-allan
Copy link
Member

I'll give this a final once over today and add my comments.

'3rdparty/jquery-ui-1.8.19/jquery-ui-1.8.19.custom.css',
'webgateway/css/omero_image.css',
'3rdparty/panojs/panojs.css',
#'webgateway/css/ome.iehacks.css',
Copy link
Member

Choose a reason for hiding this comment

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

If this is not needed it should be removed. If there's a reason it's commented out, that reason should be documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tested with and without ome.iehacks.css and I do not see any difference in IE9. I am not sure about usage of that as is only IE9 specific https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroWeb/omeroweb/webgateway/templates/webgateway/viewport/omero_image.html#L37. Removing that line for now

@chris-allan
Copy link
Member

All looks great.

Other than the above comment my only other question is the addition of:

  • components/tools/OmeroWeb/omeroweb/webgateway/static/3rdparty/jquery-ui-1.8.19/ui/minified/jquery-ui.min.js

This file was added in 9a6fb54.

Did you minify this yourself from ui/jquery-ui.js @aleksandra-tarkowska or is the minified file just from the 1.8.19 ZIP?

@atarkowska
Copy link
Member Author

@chris-allan that is from jquery-ui package. I haven't minified anything else then omeroweb.viewer.min.css and omeroweb.viewer.min.js

@atarkowska atarkowska closed this Apr 1, 2014
@atarkowska atarkowska reopened this Apr 1, 2014
@chris-allan
Copy link
Member

Cool. Looks great to me. Ready to merge.

👍

joshmoore added a commit that referenced this pull request Apr 2, 2014
@joshmoore joshmoore merged commit 30f875e into ome:develop Apr 2, 2014
@atarkowska
Copy link
Member Author

--no-rebase

@atarkowska atarkowska deleted the minify_media_viewer_only branch April 2, 2014 14:21
@sbesson sbesson added this to the 5.1.0-m1 milestone Oct 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants