-
-
Notifications
You must be signed in to change notification settings - Fork 584
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
layout interact controls #7379
Comments
comment:1
Here is a patch to the sagenb directory which allows for the following:
|
Attachment: trac-7379-interact-table-layout.patch.gz FOR SAGENB REPOSITORY |
Attachment: trac-7379-decorator-defaults.patch.gz FOR SAGE DEVEL REPOSITORY |
comment:4
Two patches are attached: one for sagenb, and one for sage devel repository. CCing timdumol (naturally, for the notebook/decorator stuff), and mhansen (for the decorator trickery) and was |
Author: Jason Grout |
comment:5
CCing mhampton, who has expressed interest in looking at this patch before. |
comment:6
See #8959 for further extensions of this idea that allow arbitrary HTML in the layout. |
comment:7
The patch at #8959 doesn't quite allow the user to specify arbitrary HTML yet, but does allow the controls to be placed on any of the four sides of the interact. Please, please review the patch at #8959 after reviewing this patch. The patch there extends the functionality of the patch here, and should be easy to review after you've looked at this patch. |
comment:8
This is great, thanks very much for working on it. The only problem I've seen so far is the following doctest failure for the notebook:
I'll try to do a little more testing and look at #8959 this weekend. |
comment:9
mhampton: that's a corner case in the
|
comment:10
The reason auto_update was included in the function arguments, rather than the However, with this patch is a decorator that transparently and easily takes care of the objection. Are there any objections now to moving the auto_update argument up to the Mike? William? You guys were the ones that objected to |
comment:11
I think it's fine with the decorator defaults patch. |
comment:12
I just posted a long message to sage-notebook for a vote. |
comment:13
I'm posting a fix to the auto_update doctest that Marshall mentioned on #8959. |
comment:14
OK, with the fix in 8959 I see no problems with this. I've tested a bunch of interacts from the wiki, and there seem to be no back-compatibility issues. So I think I can give this a positive review. |
Reviewer: M |
comment:15
The patch causes the followinger doctest error:
Marking this as "needs work." |
comment:16
In the two comments above yours, it says that the doctest is fixed in #8959. Did you apply that patch as well? |
comment:17
I didn't notice the comment. Sorry! The doctests do pass now. Marking as positive review (we really do need to add a pathway from needs_info to positive review). |
Changed reviewer from M to Marshall Hampton |
Merged: SageNB 0.8.1 |
Changed merged from SageNB 0.8.1 to sage-4.5.rc1 |
comment:20
Merged the sage-devel patch in sage-4.5.rc1. |
A user should be able to specify a layout for interact controls.
CC: @qed777 @robert-marik @TimDumol @mwhansen @williamstein @sagetrac-mhampton
Component: notebook
Author: Jason Grout
Reviewer: Marshall Hampton
Merged: sage-4.5.rc1
Issue created by migration from https://trac.sagemath.org/ticket/7379
The text was updated successfully, but these errors were encountered: