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

WebAgg: extracted figure_div style into css and changed layout #2825

Merged
merged 2 commits into from Feb 28, 2014

Conversation

vmarkovtsev
Copy link
Contributor

Set #figure-div display style to inline-block to use free space on the page
more effectively. See http://www.vanseodesign.com/blog/demo/inline-block/.
Additionally, set margin to 10px around every figure, so padding-bottom
becomes unnecessary.

Set #figure-div display style to inline-block to use free space on the page
more effectively. See http://www.vanseodesign.com/blog/demo/inline-block/.
Additionally, set margin to 10px around every figure, so padding-bottom
becomes unnecessary.
@vmarkovtsev
Copy link
Contributor Author

Please note that I have no idea why my patches break Travis build. It seems broken already.

@tacaswell
Copy link
Member

You broke pep8 compliance (added a semi-colon to a line in webagg_backend.py)

@vmarkovtsev
Copy link
Contributor Author

Damn, it was hard to interleave C++, Javascript and Python yesterday. Fixed.

@@ -14,7 +14,7 @@
$(document).ready(
function() {
var main_div = $('div#figures');
var figure_div = $('<div style="padding-bottom: 12px"/>')
var figure_div = $('<div id="figure-div"/>')
Copy link
Member

Choose a reason for hiding this comment

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

I am barely literate in css. To check my understanding, this changes from doing the styling in-line (bad) to doing the styling in the css file (good)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. But I changed the style, too.

Copy link
Member

Choose a reason for hiding this comment

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

I know, I just just picking up on the style vs id change.

@tacaswell
Copy link
Member

The logging changes seem like much bigger than the style changes.

@mdboom I think this one needs your attention.

@tacaswell tacaswell added this to the v1.4.0 milestone Feb 21, 2014
@tacaswell
Copy link
Member

And don't worry about the 3.2 fail, that is a known problem (that I really should fix as I think I broke it).

@mdboom
Copy link
Member

mdboom commented Feb 21, 2014

What's the motivation for the new logging configuration? Would a "silent" kwarg in fact meet your needs better? I'm wary of adding more rcParams...

@vmarkovtsev
Copy link
Contributor Author

I need to have a way to tell WebAgg to use logging instead of print-ing. I am unaware of any other way of doing this. Always printing messages is bad for a library. I would just replace print with logging, but I guess you will find it too radical and inacceptable.

@vmarkovtsev
Copy link
Contributor Author

Shutting WebAgg up is bad, either. My production app keeps track of all the events and if WebAgg reports some unexpected text, it requires attention.

@mdboom
Copy link
Member

mdboom commented Feb 21, 2014

I was suggesting the "silent" kwarg would merely silence these specific messages, not everyhing, and it would be a kwarg rather than an rcParam, since if you are starting the class directly, presumably you know that you don't want these messages. Using global state (an rcParam) for that is too invasive. These messages are really just conveniences for a user starting up the server at the console, and needing instruction to open the item in their web browser. It won't ever be useful as a log message, since it doesn't really indicate unusual behavior.

My question was more aimed at -- how are you calling WebAgg, and why is logging these messages helpful to you? I think you've indicated that they are not helpful, but you'd like to continue to get any unusual warnings or exceptions.

@tacaswell tacaswell removed this from the v1.4.0 milestone Feb 21, 2014
@vmarkovtsev
Copy link
Contributor Author

Okay, seems that I worked out a solution which should satisfiy everybody. No rcParam-s. Old behavior is preserved. I am able to hack "WebAgg" logger to use a custom handler.

The thing in logging "user messages" is to prove everything is working correct. You see, my application is asynchronous and if something blocks inside WebAgg, it goes dead. It helps to diagnose.

@tacaswell
Copy link
Member

@mdboom see the code posted in #2824

@mdboom
Copy link
Member

mdboom commented Feb 24, 2014

Sorry, I'm still not crazy about pulling in the logging module (which we don't currently use anywhere else) just for this. Can you not replace sys.stdout with whatever you want instead, without having to edit matplotlib?

@WeatherGod
Copy link
Member

@mdboom, we really do need to start embracing the logging module. I use it all the time for my work projects, and it is fantastic. I suggested as much back when we were talking about issues with matplotlib catching certain command-line arguments for triggering "logging" messages. I see no reason why we shouldn't start doing that now.

@mdboom
Copy link
Member

mdboom commented Feb 24, 2014

I don't have a problem with thoughtfully moving to using the logging module, but it should have a MEP. There's sort of ad hoc logging in matplotlib already, because it predates the stdlib logging module -- but I don't want to do it as part of this PR, where I still don't think there's a good case for it.

@tacaswell
Copy link
Member

I am agree with both. We should do it, but not ad-hoc in this PR. I suspect we want to be able to set the logging handlers/levels at the module level.

    If a Tornado ioloop is already running before WebAggApplication.start() is called,
    it should be re-used instead of trying to start it again. This allows one to
    use WebAgg in his/her external Tornado application.
@vmarkovtsev
Copy link
Contributor Author

Okay, I got it.

@efiring
Copy link
Member

efiring commented Feb 24, 2014

Logging module: I use it, too, but care will be required because it's global. It's a bit tricky.

@WeatherGod
Copy link
Member

Right, that is why a library needs to treat its loggers that it sets up
like an API. It is up to the script-writer to decide how to use those
particular loggers that are provided by the library. The end-user adds
handlers that are appropriate for its own use, the library merely emits
useful messages according to logging levels. By default, all loggers are
off (although, if you use logging.basicConfig(), it will turn everything
on).

On Mon, Feb 24, 2014 at 2:30 PM, Eric Firing notifications@github.comwrote:

Logging module: I use it, too, but care will be required because it's
global. It's a bit tricky.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2825#issuecomment-35925220
.

@tacaswell tacaswell added this to the v1.4.0 milestone Feb 25, 2014
@tacaswell
Copy link
Member

@mdboom Can you push the button on this one if you are happy?

@vmarkovtsev
Copy link
Contributor Author

@mdboom Are you happy? :)

mdboom added a commit that referenced this pull request Feb 28, 2014
WebAgg: extracted figure_div style into css and changed layout
@mdboom mdboom merged commit 9b78b9e into matplotlib:master Feb 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants