-
Notifications
You must be signed in to change notification settings - Fork 282
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 extra buttons from liferay-portal #954
Conversation
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.
Only a partial look at this so far. Will continue a bit later.
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.
Some more comments after reading through the whole thing. Pretty great that you were able to make such a large change without anything getting busted. :-)
var instance = this; | ||
|
||
var focusAltEl = function () { | ||
ReactDOM.findDOMNode(instance.refs.refAltInput).focus(); |
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.
Use of findDOMNode
is deprecated (or will be soon) — the suggestion is to use refs instead.
) | ||
} else { | ||
active = false; | ||
} |
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.
I wonder if all this would read better with every()
? Something like:
return styleConfig.attributes.class.split(' ').every(
className => elementPath.lastElement.hasClass(className)
);
Hey @jbalsas and @wincent,
This add the extra buttons that we had in
liferay-portal
.There are probably still a few things to clean up and maybe discuss for this to be totally finished but I thought I'd send you the PR so we can start the conversation.
On the top of my head these are the main things that need to be finished or at least that you should be aware of.
button-camera-image.jsx
component has been fixed, I discovered it was no longer working in Chrome or Safari while working on this task.button-link-browse.jsx
component was usingAUI
(I removed that), we talked about either emitting an event or allowing passing a callback, it might be a good time to talk about it together and decide what which solution seems "right".embedurl
plugin work outside the portal, but @jbalsas and I already agreed that we would have a look at it.ae-svg-icon
css class with @marcoscv-work but please let us know what you think@wincent if you see anything "bad" or crazy don't hesitate your feedback is more than welcome.
Thanks!