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

Master Slide & Layout Slides #161

Closed
wants to merge 27 commits into from

Conversation

kajda90
Copy link

@kajda90 kajda90 commented Aug 22, 2017

Hi @gitbrent,

I've implemented the master slide and slide layout solution I've proposed in the issue #146. It's kinda huge pull request so I'll try to focus on the major changes and important parts in the following list:

API Notes

  • New functions are described in README, usage should be easy and obvious. Briefly, each slide is attached to a slide layout (that can be used by several slides). A slide layout is attached to the master.
  • Only a single master file is supported.
  • Backward compatibility was fully preserved. Thus, to use this feature, pptx.useProperMasterLayout needs to be called before saving presentation – this ensures that user intentionally work with the new approach provided by the feature.
  • I did not find any tests but I've checked the backward compatibility at least by the demo file that is included in the repo.

Implementation Notes

  • Defining a slide, a slide layout and the master slide expects the same configuration used for defining master the old way. Using same definitions results in using same functions for creating internal objects. For this reason, a lot of functions that had been defined within the slide object have moved to a new global object gObjPptxGenerators. But still, the slide object (compared to layout or master slide) has extra functions (such as addTable) – however, these could be moved to gObjPptxGenerators as well.
  • Defining multiple types of slides means handling multiple relations. Only a slide could refer to images or charts before (even if master was used). But now, slides, slide layouts and the master slide can include externals. Therefore, creating relation files, excel worksheets, encoding images etc. have been refined and is shared now. Extra global counters for images and charts have been added to generate their filenames consistently without collisions.
  • Number series used for rIds also changed. There had been only a single static relation before – link to the master file in layout files and link to a layout in a slide file, so the number series for externals could have started at 2 safely. But now, even slides and the master can contain various amount of externals and static relations. Thus, dynamically added externals get rIds at first and then static relations are indexed. That means, in a layout file that includes an image, link to the master file gets rId = 2 and the image gets rId = 1. This has no visual effect, of course.

Formal Notes

In touched functions, I have

  • replaced $.each(array, function(idx, el) { ... }) by array.forEach(function(el, idx) { ... }) because I've noticed you were using both but it's not necessary to use jQuery alternative when there's the native one;
  • replaced chatty object definitions where an array is accessed multiple times (ineffective) by listing all object properties at once.

I'm not sure I've mentioned all I should have so if you come across any unclear parts (whether in my description or in the code), ask me please. In closing, I'd some suggestions:

  • Please, consider changing the code to more objective one. Slide methods are defined every time a new slide is being added; but in fact, they could be inherited. The list of slides is stored as a property of the global object, but it could a member of PptxGenJS class. It would be much more handy if slide objects and slides themselves were defined as classes. I haven't done any such changes because the PR would be probably really huge 🙂
  • Try to specify coding standards, not only for you but even for your contributors. I've noticed there are many forms of notation used for the same, spaced brackets and non-spaced brackets etc. (https://eslint.org/)

Michal Kacerovsky added 25 commits August 28, 2017 09:11
…ayout files can be passed to the presentation. Otherwise, works as originally.
…amically added to master slide. Shape support added.
…errenced relations are added (layouts, masters, themes)

+ Master slide is generated by JSON
Chatty object creating changes to a single assignment.
$.each -> array.forEach
Refactoring nomenclature
Conditions on gObjPptx.masterSlide removed (always defined, but empty by default)
@gitbrent gitbrent self-requested a review August 29, 2017 04:14
@gitbrent gitbrent self-assigned this Aug 29, 2017
@gitbrent gitbrent added this to the 1.8.0 milestone Aug 29, 2017
@kajda90
Copy link
Author

kajda90 commented Aug 31, 2017

Allowing the new master-layout approach has changed. The method useProperMasterLayout has been replaced by passing true as the first argument of the constructor. Plus, the library exports besides the instance of PptxGenJS even its factory to be able to create instances in Node as well.

@gitbrent
Copy link
Owner

gitbrent commented Sep 1, 2017

Whoa, i'm already doing integration work on this. Please stop changing the PR.

PR-161 WIP

@kajda90
Copy link
Author

kajda90 commented Sep 1, 2017

I'm sorry, I did not noticed it's in progress. This was just a little change, however if any other will appear, I'll create another PR conditioned by this one.

@gitbrent
Copy link
Owner

gitbrent commented Sep 3, 2017

Thanks @kajda90 !

Great work on the creation of actual Slide Master Layouts. This is a major improvement to the generated presentations and is easier to use than the old method.

Note that i made a few changes that will impact your code when you start using the current version...

Basically, i removed the legacy Master Slides (pptxgen.masters.js) then took your work and made it the default. :-) So, you won't need the flag - layout style is always used - plus, I collapsed the two methods you created together so every slide master/layout needs a name and is referenced in the add slide method.

It's a change for you, but hopefully something easy and concise for the general user population. The README and examples have been updated, but here's a summary:

pptx.defineSlideMaster({
  title: 'MASTER_SLIDE',
  bkgd:  'FFFFFF',
  objects: [
    { 'line': { x: 3.5, y:1.0, w:6.00, line:'0088CC', line_size:5 } },
    { 'rect': { x: 0.0, y:5.3, w:'100%', h:0.75, fill:'F1F1F1' } }
  ]
});

var slide = pptx.addNewSlide('MASTER_SLIDE');

@gitbrent gitbrent closed this Sep 3, 2017
gitbrent pushed a commit that referenced this pull request Sep 3, 2017
@kajda90
Copy link
Author

kajda90 commented Sep 4, 2017

Hi @gitbrent !
I am glad you set this approach as default :) However, as for the collapsed methods, I'm not sure they can be collapsed. How can I define master slide and slide layout now? They are two different things. Master slide (or master slide layout) works as the parent of all slide layouts and so, they inherit all objects used and props used in master slide (layout). Now, when the methods addMasterSlide and addSlideLayout are collapsed, I'm not able to define master slide layout, but just slide layout. Or did I miss something? :)

There's a use case, consider the following:
I'm creating a presentation where some of the slides contain slide number and some of them don't. This leads to two layouts, let's call them numbered and not-numbered. In the master slide layout, I define position of the slide number, because it's a general property of a slide that should be inherited by all slides/layouts. Then, in a particular layout, I define whether to show slide number or not. Using this approach, slide number won't be visible in slides following not-numbered layout, but still, users has an option to show it and if they do so, it appears in the place defined by master slide layout.

And you removed the support for creating PptxGenJS instances in Node? (I mean the factory.)

@gitbrent
Copy link
Owner

gitbrent commented Sep 6, 2017

Hi @kajda90 ,

Thanks for following up. :-)

The current design does only create Master Slide Layouts - and not a parent Slide Master (it's just blank/white). I did that because i wanted to provide a simple interface for new users and have a similar mechanism for current users - creating a thing to put another thing under seems like a lot of work to some users.

I plan to add back the ability to create an actual Master Slide, then add these Layouts under it, but probably at 2.0. I'm more keen to take big steps with features like this and not giant leaps.

Lastly, the factory and that other item (or two?) that were added came after i branched and you said you'd open another PR, so I did not attempt to integrate them. I'm very interested in the factory though! Please provide a demo showing how to use it - i'm not a nodeJS expert. :-)

Thanks again for all the hard work. I really appreciate your contributions.

@kajda90
Copy link
Author

kajda90 commented Sep 6, 2017

Hi @gitbrent ,

Aha, okay, it's up to you.

As for the factory, I can create a demo of course and add missing information to readme. Should I create the factory as a new PR? I guess so.
I don't think there were any other implementations added after you branched :) I just only used constructor argument instead of calling userProperMasterLayout, but since you set the new approach as default, it's meaningless :)

@gitbrent
Copy link
Owner

gitbrent commented Sep 7, 2017

Yes, please PR and document the factory. We have a large Node user base, and i know they'd appreciate your contribution.

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.

2 participants