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

Buttons' visibility #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Buttons' visibility #28

wants to merge 6 commits into from

Conversation

lcaprini
Copy link

Extend buttons' visibility to all available buttons with provider configuration and directive override

@jasny
Copy link
Member

jasny commented Feb 13, 2017

Improvement, but BC break

@lcaprini
Copy link
Author

I restored all previous attributes to avoid backward compatibility.

Copy link
Member

@jasny jasny left a comment

Choose a reason for hiding this comment

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

Approved. Need to test before merge

@jasny
Copy link
Member

jasny commented Mar 14, 2017

@moesjarraf Can you pick up testing and merging this?

@moesjarraf
Copy link
Member

moesjarraf commented Mar 14, 2017

I don't think a lot of the code in this PR is necessary. This angular module works based on mozilla/pdf.js. Which means that if they add new buttons, we will have to update the code aswell since you added an object property for every button. That object is not necessary, even for the default behaviour, because by default all the buttons are displayed anyway.

It would be best to just add a list of buttons to the readme, and through an attribute we can specify an array of id's that should be disabled/removed.

You don't have to check whether the id exists in an object, just do:

// psuedo code
var button = document.getElementById(key);

if (!button) {
  return;
}

// continue logic
... 

So add the following to the readme, as you put in the code:

buttons: {
  sidebarToggle : true,
  viewFind : true,
  previous : true,
  next : true,
  pageNumberLabel : true,
  pageNumber : true,
  numPages : true,
  zoomOut : true,
  zoomIn : true,
  scaleSelectContainer : true,
  presentationMode : true,
  openFile : true,
  print : true,
  download : true,
  viewBookmark : true,
  secondaryToolbarToggle : true
}

@jasny
Copy link
Member

jasny commented Apr 2, 2017

@lcaprini Can you please make the suggested change. Buttons should be visible unless they are configured not to be visible. This makes adding defaults for each button unnecessary.

don't worry about the dist conflict

Updated demo with buttons example
@lcaprini
Copy link
Author

lcaprini commented Apr 3, 2017

@jasny @moesjarraf I removed default buttons list to accept all actual and future buttons.
Also I've updated demo with buttons configuration example

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants