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

Fullscreen Support #99

Merged
merged 33 commits into from
Sep 3, 2014
Merged

Fullscreen Support #99

merged 33 commits into from
Sep 3, 2014

Conversation

lodev09
Copy link
Contributor

@lodev09 lodev09 commented Aug 29, 2014

This is a beta support for fullscreen. I was working on my project and would be nice to have a fullscreen feature so I added one. 😄

Here is sample plunk: http://embed.plnkr.co/atyuBnVNktl8YSfdfPJT/preview

@@ -368,8 +388,26 @@
this.$editor.addClass('active')
}

this.$editor.append('\
<div class="md-fullscreen-controls">\
<a href="#" class="switch-theme" title="Switch themes"><span class="glyphicon glyphicon-adjust"></span></a>\
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add the other supported icon libs like fontawesome and fontawesome v3 like the rest of the buttons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was thinking of that too.

@acrobat
Copy link
Contributor

acrobat commented Aug 29, 2014

👍 Great feature!

I think you should rebase this pull-request against upstream-master because, you have lot's of changes here that were already commited on master, that way we won't lose changes in the progress of merging this PR in master! + it's quite difficult to see what changes exactly are related to this PR. A rebase should solve this

@lodev09
Copy link
Contributor Author

lodev09 commented Aug 29, 2014

I already rebased this and made updates based from the current v2.6.0's code

@acrobat
Copy link
Contributor

acrobat commented Aug 29, 2014

hmm strange.. the commit tab shows a lot of commits which are already merged in master

@lodev09
Copy link
Contributor Author

lodev09 commented Aug 29, 2014

Indeed. But the current version I was working on was 2.6.0.

@toopay
Copy link
Member

toopay commented Aug 29, 2014

@lodev09 your fork hash display This branch is 23 commits ahead, 28 commits behind warning. git merge should resolve it.

Conflicts:
	js/bootstrap-markdown.js
@lodev09
Copy link
Contributor Author

lodev09 commented Sep 1, 2014

Try now @toopay

@acrobat
Copy link
Contributor

acrobat commented Sep 1, 2014

@lodev09 please fix the classes for the switch-theme and exit-fullscreen so we add support for all the icon fonts. Thanks!

@lodev09
Copy link
Contributor Author

lodev09 commented Sep 1, 2014

Added.. Below are the default configuration

...
fullscreen: {
  enable: true,
  icons: {
    fullscreenOn: {
      fa: 'fa fa-expand',
      glyph: 'glyphicon glyphicon-fullscreen',
      'fa-3': 'icon-resize-full'
    },
    fullscreenOff: {
      fa: 'fa fa-compress',
      glyph: 'glyphicon glyphicon-fullscreen',
      'fa-3': 'icon-resize-small'
    },
    switchTheme: {
      fa: 'fa fa-adjust',
      glyph: 'glyphicon glyphicon-adjust',
      'fa-3': 'icon-adjust'
    }
  },
  theme: '' // you can put "dark" for now
}
...

@toopay
Copy link
Member

toopay commented Sep 1, 2014

@lodev09 Several notes :

  • Reorder the fullscreen to appropriate place. Any methods which prefixed with __ are intended to use internally, while the other are exposed via markdown instance.
  • If we're gonna expose this new function, then lets make it uniformal with other. showFullscreen is fine (similar with showPreview)
  • Hence by nature, you will also need to maintain fullscreen state as global variable. Adding isFullscreen into class property and...
  • Adding hook onFullscreen seems to be logical
  • Also, can you ships the minified version instead append your changes by hand at css/bootstrap-markdown.min.css ?

Lets adding something like this properly so we doesn't break the clarity of the code as it getting bigger.

Cheers.

@lodev09
Copy link
Contributor Author

lodev09 commented Sep 1, 2014

I've renamed it setFullscreen since it's taking an argument bool mode arg.

@toopay
Copy link
Member

toopay commented Sep 1, 2014

Wonderful. Thanks.

One more point thou. I'm still not sure whether theme related code are required for this PR. I prefer to have the vanilla version of fullscreen feature to keep the code not having very firm ideas about how things ought to be done. I can see you're providing an option that makes it configurable, but i'm not seeing that should be within core logic, instead we can provides hook for that (onFullscreen), and let the user do whatever they need to do. Thought?

@lodev09
Copy link
Contributor Author

lodev09 commented Sep 2, 2014

I guess your right.. I just thought we could incorporate additional theme in the future (by just adding stuff in css) and let that property be the default theme of the fullscreen layout

@toopay
Copy link
Member

toopay commented Sep 2, 2014

Great. Lets remove any theme related block and this PR can be merged.

@lodev09
Copy link
Contributor Author

lodev09 commented Sep 2, 2014

@toopay done.

I've also added a block into the keyup event for escape. It closes the fullscreen when pressed.

toopay added a commit that referenced this pull request Sep 3, 2014
@toopay toopay merged commit fa9a64e into refactory-id:master Sep 3, 2014
@toopay
Copy link
Member

toopay commented Sep 3, 2014

Thanks @lodev09 !

@lodev09
Copy link
Contributor Author

lodev09 commented Sep 3, 2014

No problem!

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.

5 participants