-
Notifications
You must be signed in to change notification settings - Fork 113
Buttons' visibility #28
base: master
Are you sure you want to change the base?
Conversation
Added button’s visibility override in directive
Improvement, but BC break |
I restored all previous attributes to avoid backward compatibility. |
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.
Approved. Need to test before merge
@moesjarraf Can you pick up testing and merging this? |
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 // 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:
|
@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 |
Updated demo with buttons example
@jasny @moesjarraf I removed default buttons list to accept all actual and future buttons. |
Extend buttons' visibility to all available buttons with provider configuration and directive override