Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

split template: add top/bottom orientation #372

Closed
peterpeterparker opened this issue Sep 28, 2019 · 15 comments · Fixed by #406
Closed

split template: add top/bottom orientation #372

peterpeterparker opened this issue Sep 28, 2019 · 15 comments · Fixed by #406
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest-accepted Good for Hacktoberfest studio "Studio" is the codename for the web editor templates Issue or feature regarding the templates
Milestone

Comments

@peterpeterparker
Copy link
Contributor

Description

The "split" template (https://docs.deckdeckgo.com/slides/split), display currently the content in two columns "left/right".

The idea would be to add a new options in order to be able to display the content in two rows "top/bottom"

This enhancement should find place in the related slide web component.

Once implemented, the option could be added our editor ("studio").

Questions

  1. About the implementation in the editor, for simplicity reason, should we add the "bottom/top" as an option of the split template (new button for option which link to a right popover with the option) or should we "just" add two different templates style in the list of selectable template ("when user add a new slide")

  2. Should we go one step further and add more options to the templates for example to be able to display "left/right" on desktop/projector and for the same content "top/bottom" on mobile/tablet. Maybe, to ease the development, if this is cool, we could do that later in another issue?

@peterpeterparker peterpeterparker added enhancement New feature or request templates Issue or feature regarding the templates studio "Studio" is the codename for the web editor good first issue Good for newcomers labels Sep 28, 2019
@timrodz
Copy link
Contributor

timrodz commented Sep 30, 2019

Heya! I'd be happy to take on this issue. Just found about DeckDeckGo today and I must say it looks great! The docs are superb too. 🕺

@peterpeterparker
Copy link
Contributor Author

Awesome @timrodz and happy to here the docs is cool 👍

It would be super neat if you could give a hand.

You could find the source of the "split slide/template" in https://github.com/deckgo/deckdeckgo/tree/master/webcomponents/slides/split

git clone your-fork-of-deckdeckgo
cd deckdeckgo/webcomponents/slides/split
npm install
npm run start

http://localhost:3333/

Not the most really beautiful test page though ;)

Ping me if you have any questions or let me know how I could help.

@timrodz
Copy link
Contributor

timrodz commented Oct 1, 2019

Thank you, @peterpeterparker! I'll have a go at it 😄

@timrodz
Copy link
Contributor

timrodz commented Oct 9, 2019

Hi @peterpeterparker, I've had a look at the codebase and had a few questions / thoughts I'd like to verify - I haven't used Stencil before, so I'm quite unfamiliar with the codebase or the way Stencil works.

  • According to the docs, every webcomponent works as separate npm packages, correct?
  • I notice the use of namespacing for specifying settings such as LocalJSX, which I assume is the code that developers will inject to match their own implementations.
  • For adding the top/bottom split, I'm thinking of creating either:
    • A setting so the user can specify 'vertical'/'horizontal' and it'll render different HTML/CSS | Seems like adding the new split code to deckdeckgo-slide-split.scss would be good enough
    • A new interface along HTMLDeckgoSlideSplitElement that will have the changes for the vertical split

@peterpeterparker
Copy link
Contributor Author

peterpeterparker commented Oct 10, 2019

  • Yes

  • I don't understand your question regarding "namespacing and LocalJSX". Could you point out a line of code you are referencing?

  • I think a setting respectively a new @Prop() is the way (your first option, agree with you). Then, according the value, we could apply a class to the container in the render() method and finally implement the different display in the scss file

@timrodz
Copy link
Contributor

timrodz commented Oct 10, 2019

Thanks for your answers!

I don't understand your question regarding "namespacing and LocalJSX". Could you point out a line of code you are referencing?

Let me rephrase my question: What's the relationship between components.d.ts and deckdeckgo-slide-split.tsx? Similarly, how do these files talk to each other (assuming they do)

If I take the boolean customBackground from components.d.ts, I can notice it's referenced inside Components::DeckgoSlideAuthor and LocalJSX::DeckgoSlideAuthor of the same script.

Screen Shot 2019-10-10 at 9 50 33 PM

Screen Shot 2019-10-10 at 9 50 47 PM

It is also referenced as a @Prop() inside deckdeckgo-slide-split.tsx, so I'm assuming to define a new prop I'd have to follow similar steps.

Screen Shot 2019-10-10 at 9 53 36 PM

I think a setting respectively a new @Prop() is the way (your first option, agree with you). Then, according the value, we could apply a class to the container in the render() method and finally implement the different display in the scss file

Coming from React, I can see accessing variables inside Render() like so:

Screen Shot 2019-10-10 at 9 55 23 PM

Based on Stencil's docs, I can infer that I can simply use JSX to change the class names and then I'll go along and toy around with scss until I get the desired results.

@peterpeterparker
Copy link
Contributor Author

components.d.ts is the component's definition file, you could safely ignores it as it is automatically generated by Stencil at build time (npm run start or npm run build).

Exactly you could define a new@Prop() as the other. We don't necessary need the reflectToAttr right now, as you wish.

Based on Stencil's docs, I can infer that I can simply use JSX to change the class names and then I'll go along and toy around with scss until I get the desired results.

Exactly, you read my mind 👍

P.S.: Note in Stencil JSX class is class, not className as in React

@timrodz
Copy link
Contributor

timrodz commented Oct 10, 2019

Awesome, thank you! I'll have a jam during the weekend - It's almost there! 🕺

@peterpeterparker
Copy link
Contributor Author

Awesome! Ping me anytime if you've got questions or need help.

@peterpeterparker peterpeterparker added the hacktoberfest-accepted Good for Hacktoberfest label Oct 10, 2019
@timrodz
Copy link
Contributor

timrodz commented Oct 11, 2019

Getting there! Just need to add diff properties to the .scss file

Screen Shot 2019-10-12 at 9 46 11 AM

My approach has been to make it so that we touch as little code as possible:

@Prop() isVertical: boolean = false;

render() {
  const verticalAttr = this.isVertical ? '-vertical' : '';
  return <Host class={{'deckgo-slide-container': true}}>
    <div class={`deckgo-slide${verticalAttr}`}>
      <slot name="title"></slot>
      <div class="deckgo-slide-split deckgo-slide-split-start"><slot name="start"></slot></div>
      <div class="deckgo-slide-split deckgo-slide-split-end"><slot name="end"></slot></div>
      <slot name="notes"></slot>
      <slot name="actions"></slot>
      <slot name="background"></slot>
    </div>
  </Host>;
}

Looks like I'll probably have to add verticalAttr to an alternative for deckgo-slide-split so that it can properly calculate the height - What do you think?

@timrodz
Copy link
Contributor

timrodz commented Oct 11, 2019

Current implementation:

deckgo_001

It's able to split 50/50 vertically - But I need to take the paddings in the formula, so might have to re-calculate height and width accordingly:

div.deckgo-slide-vertical {
  @extend div, .deckgo-slide;
  flex-flow: column wrap;
}

div.deckgo-slide-split-vertical {
  @extend div, .deckgo-slide-split;
  // TODO
}

Screen Shot 2019-10-12 at 10 24 06 AM

@timrodz
Copy link
Contributor

timrodz commented Oct 11, 2019

Added colours to show the differences between both splits - If you think this is fine, I'll make a Pull Request and we can discuss the code further.

Horizontal (Default)

Vertical

@peterpeterparker
Copy link
Contributor Author

It looks good 👍 thx!

Furthermore you teach me something, I never used @extend before, thx :)

about size, yes the size is given by the class .deckgo-slide which is defined in the util class @deckdeckgo/slide-utils (

)

Super cool, go for the PR 🚀

@peterpeterparker
Copy link
Contributor Author

Thx you for the neat PR @timrodz 🙏

peterpeterparker added a commit that referenced this issue Oct 13, 2019
… editor, therefore already reflect the attribute
peterpeterparker added a commit that referenced this issue Oct 13, 2019
… editor, therefore already reflect the attribute
@peterpeterparker
Copy link
Contributor Author

peterpeterparker commented Oct 13, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest-accepted Good for Hacktoberfest studio "Studio" is the codename for the web editor templates Issue or feature regarding the templates
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants