-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Presentations wrapper #739
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
Conversation
Re: #712 |
The presentations wrapper can now do something. 🎉 Check out the example below:
and the presentation is live here General Rules for the Syntax:
Points for Improvement/Discussion:I'm debating whether we should stick pretty closely to the remark syntax. It uses Personally, I'd like both: to be able to customize where the boxes are going to go in the layout, and if not specified they can default to one of a few themes. Maybe something like .left[some text here, (15, 30)] would be okay, as it puts all the power of determining the exact coordinates in the users' hands. Thoughts? Holding off on tests until we have a solid gameplan for the final design. 🏈 |
Could this be either pixels or a percent-from-left? As a test, I'd love to see how close we could get to this presentation using the markdown syntax: As another test, maybe one of these slide decks from @cpsievert that used http://rmarkdown.rstudio.com/ioslides_presentation_format.html? |
Yes! Yes! on it! 👍 |
I like percent over pixels.
|
Here's my suggestion for how we treat the presentations: For a first pass, we should force all text, codepanes, images, and plotly charts (except for headers that start with
where the alignment command is read like "place 45% from left, 12% from top". I think this offers a lot of customizable options, even though it's not identical to the Thoughts? @chriddyp @jackparmer @cpsievert |
👍 sounds find to me. Thanks Adam!
…On Thu, May 11, 2017 at 12:58 PM, Adam Kulidjian ***@***.***> wrote:
Here's my suggestion for how we treat the presentations:
For a first pass, we should force all text, codepanes, images, and plotly
charts (except for headers that start with #, ##, ###, ####) to start
with the following syntactical bit:
.left45top12[all of your text goes here]
where the alignment command is read like "place 45% from left, 12% from
top".
I think this offers a lot of customizable options, even though it's not
identical to the remark format.
Thoughts? @chriddyp <https://github.com/chriddyp> @jackparmer
<https://github.com/jackparmer>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#739 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABx4al6Pg6LvWYo2g3tE-nQO63OpI4RPks5r42hkgaJpZM4NFePJ>
.
|
We should be able to replicate those other presentations you linked me |
CHANGELOG.md
Outdated
@@ -2,6 +2,10 @@ | |||
All notable changes to this project will be documented in this file. | |||
This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|||
## [2.1.1] - 2017-10-20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll make this 2.2.0 based on adding new features: #852 (comment)
plotly/plotly/plotly.py
Outdated
|
||
:param (dict) presentation: the JSON presentation to be uploaded. Use | ||
plotly.presentation_objs.Presentation to create the presentation | ||
from a string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what "from a string" means here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that you are defining a variable as a string and passing that into the presentations function to generate the JSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Maybe "... to create presentations from a Markdown-like string"?
plotly/plotly/plotly.py
Outdated
:param (str) sharing: can be set to either 'public', 'private' | ||
or 'secret'. If 'public', your presentation will be viewable by | ||
all other users. If 'private' only you can see your presentation. | ||
If 'secret', the url will be returned with a sharekey appended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if folks outside of Plotly will know what "sharekey" is. Could this be: "The url will contain a random, hard-to-guess string appended to it. Anyone with the url may view the presentation."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about i keep the word "sharekey" but explain what it is? I've used the term in dashboard_ops
function as well and I think it's a good term for people to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. What do you propose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: If it is set to 'secret', the url will be returned with a string of random characters appended to the url which is called a sharekey. The point of a sharekey is that it makes the url very hard to guess, but anyone with the url can view the presentation.
plotly/plotly/plotly.py
Outdated
world_readable = False | ||
elif sharing == 'secret': | ||
world_readable = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like in other wrappers, we should raise an exception if sharing
wasn't one of those strings
plotly/plotly/plotly.py
Outdated
# lookup if pre-existing filename already exists | ||
try: | ||
lookup_res = v2.files.lookup(filename) | ||
matching_file = json.loads(lookup_res.content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a lookup_res.raise_for_status()
?
'Bold': {'fontWeight': 700}, | ||
'Bold Italic': {'fontWeight': 700, 'fontStyle': 'italic'}, | ||
'Black': {'fontWeight': 900}, | ||
'Black Italic': {'fontWeight': 900, 'fontStyle': 'italic'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be "Very Bold"? Bold doesn't necessarily mean black
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are based on the naming conventions in the app itself, so I think we should keep them consistent with the pres app
|
||
elif boxtype == 'Image': | ||
props = { | ||
'height': 512, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you comment where 512
came from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this:
# the default height, width for an Image are
# both set to 512
props = { | ||
'frameBorder': 0, | ||
'scrolling': 'no', | ||
'src': text_or_url + '.embed?link=false', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this missing sharekey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not, but the embed line should not be there if there is a sharekey - I just tested that
now it checks if ?sharekey
is in the url and will not put the .embed?link=false
if that is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with a sharekey it should still have .embed
and link=false
, it will just look something like:
.embed?link=false&sharekey=....
vs just
.embed?link=false
"'left', 'top', 'width', and 'height' parameters must be " | ||
"set equal to a number (percentage) or a number with " | ||
"'px' at the end of it. For example in " | ||
"\n\n.left=10;top=50px{{TEXT}}\n\n the top left corner of " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's {{TEXT}}
mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just deleted that message. It's nested in a hidden function so we shouldn't have it at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run flake8
on this file? That'll catch unused variables like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8
didn't catch unused variables but pylint
did 👍
while line.endswith('\n') or line.endswith(' '): | ||
line = line[: -1] | ||
return line | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there are more than 1 white spaces? test
lstrip
might be a more robust solution here: https://docs.python.org/3/library/stdtypes.html#str.lstrip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent reference! I didn't know about this
markdown_string += '\n---\n' | ||
|
||
text_blocks = re.split( | ||
'\n--\n|\n---\n|\n----\n|\n-----\n|\n------\n', markdown_string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a regex like \n-{2,}\n
be more generic? Check out https://regex101.com/ for testing regexes
) | ||
list_of_slides = [] | ||
for j, text in enumerate(text_blocks): | ||
if not all(char in ['\n', '-', ' '] for char in text): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this line doing? It seems like it's trying to reject strings like \n---\n
that are used as separators?
l_brace_idx = slide_copy[left_idx:].find('{{') + left_idx | ||
properties = slide_copy[left_idx + 1: l_brace_idx].split( | ||
prop_split | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add some comments that describe what this function and this loop is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a unused function - deleting
|
||
def _top_spec_for_text_at_bottom(text_block, width_per, per_from_bottom=0, | ||
min_top=30): | ||
# TODO: customize this function for different fonts/sizes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments that describe what this function does (inputs and outputs) would be really helpful
raise exceptions.PlotlyError( | ||
"You are attempting to insert a Plotly Chart " | ||
"in your slide but your url does not have " | ||
"plot.ly in it." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the user is using plotly on-premise, then this URL won't necessarily have plot.ly
in it. It could be like plotly.acme.com
"Notice how the language that you want the code to be " | ||
"displayed in is immediately to the right of first " | ||
"entering '```', i.e. '```python'." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of hardcoding these messages, could you import them from the original file? That way, if we wanted to change these messages, we would only have to do it from one place
self.assertEqual( | ||
my_pres['presentation']['slides'][0]['children'][0]['props'], | ||
exp_pres['presentation']['slides'][0]['children'][0]['props'] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests are pretty light- it looks like there is just one test that for a simple presentation, doesn't include Plotly
, Image
, multiple slides, or any of the different layout options. Could you try to come up with a few more tests that test these different options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
my reason for making it so light was that the JSON blobs are so large anyways
but I agree we should be testing the other features too
@chriddyp Okay, all DONE! 🎉 |
thanks for making those changes! 💃 from me |
No description provided.