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

Add CustomAction toolbar action #8099

Merged
merged 7 commits into from Jul 23, 2018
Merged

Add CustomAction toolbar action #8099

merged 7 commits into from Jul 23, 2018

Conversation

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Jul 21, 2018

This is a slight diversion, but I intend to use this targetable toolbar button that executes callback code as a simple and standard means to inject JS code into selenium tests. Additionally, it seems like quite a useful feature for users in its own right.

This PR also adds an Image property, that can be set from RGB(A) NumPy arrays, PIL image objects, or from filenames of images in disk. The transformed value is always a Base64 data\image string. In this way the icon for the toolbar tool may be set to an arbitrary choice by the user.

This requires adding Pillow as a dependency, however Pillow is easily obtainable. Additionally, there was Gitter discussion about adding PNG, TIFF, etc. image glyphs, and Pillow will be needed for that work as well.

if (this.model.icon.startsWith("bk-tool-icon-"))
this.el.classList.add(this.model.icon)
else
this.el.style.backgroundImage = "url('data:image/png;base64," + this.model.icon + "')"

This comment has been minimized.

@bryevdv

bryevdv Jul 21, 2018
Author Member

Just occurs to me this is problematic, as ostensibly Image will accept other valid PIL image formats. But I'm not sure data URI's can actually be more than PNGs. Might end up simply restricting what Image accepts.

model: CustomAction

css_classes(): string[] {
return super.css_classes().concat("bk-toolbar-button-custom-action")

This comment has been minimized.

@bryevdv

bryevdv Jul 21, 2018
Author Member

This is to provide a known target for Selenium test to be able to click this button.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jul 22, 2018

Well, this is causing real test failures for bokeh png and bokeh svg tho for the life of me I have no idea why.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jul 22, 2018

Apparently PhantomJS does not support the startsWith method, that was a fun few hours...

@mattpap
Copy link
Contributor

@mattpap mattpap commented Jul 22, 2018

Apparently PhantomJS does not support the startsWith method, that was a fun few hours...

Yep, that's why we have startsWith in core/util/string.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jul 23, 2018

@mattpap thanks, I'd forgotten. Any other comments before merging?

if (startsWith(icon, "bk-tool-icon"))
this.el.classList.add(icon)
else
this.el.style.backgroundImage = "url('data:image/png;base64," + icon + "')"

This comment has been minimized.

@mattpap

mattpap Jul 23, 2018
Contributor

I would change (reverse) this logic, especially in light of #7499. Anything that starts with data: should be treated as a CSS icon and everything else should be a CSS class. This also means that icon should contain data:image/png;base64, prefix. This way we would allow for other formats and encodings.

@bryevdv bryevdv merged commit 96aa9ec into master Jul 23, 2018
2 checks passed
2 checks passed
Better Code Hub ✅ Better Code Hub approves this code
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bryevdv bryevdv deleted the bryanv/custom_action branch Jul 23, 2018
Karel-van-de-Plassche added a commit that referenced this pull request Sep 7, 2018
* add pillow as a runtime dependency

* update release notes

* add CustomAction toolbar button tool

* fix requires spec format

* don't use startsWith method, missing from PhantomJS

* use internal startsWith

* test for data:image and treat anything else as a CSS class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.