-
Notifications
You must be signed in to change notification settings - Fork 87
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 ToggleButton and ToggleButtonWithHoverText UI Components #598
Conversation
Returns a component that can be used to trigger an action like opening or closing a container. The component appears as an icon only but acts like a button.
Returns a component that can be used to trigger an action like opening or closing a container. The component appears as an icon only but acts like a button, and is wrapped with HoverText.
Adds a text element that is only displayed when the wrapped component (children) is hovered over
…ts-ui-toggle-button
…onents-ui-toggle-button
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.
It's unfortunate you've bound this pull request so tightly to two other open pulls. It means that as a reviewer, even if I thought it was ready, I wouldn't approve it.
As it stands, I can't approve this as is anyways (see comments and feedback). I'm also curious here, why you've created the ToggleButton
instead of using GB's Button or Icon Button components?
) => { | ||
return toggleCallback && ( | ||
<div | ||
id={ `${ htmlId }-${ description }` } |
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 think this should still be wrapped by the withInstanceId
HOC to make sure this is unique.
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.
As explained in #597 you do not need to wrap absolutely everything that sets an HTML id with the withInstanceId
HOC. That would be total overkill. There is nothing stopping this id from being unique, but the dev would have to do so manually, which could include an id that has been passed down through the app that was partially generated using the withInstanceId
HOC.
tabIndex = 0, | ||
} | ||
) => { | ||
return toggleCallback && ( |
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.
Should verify toggleCallback
is a function and if it isn't use warning
to inform maybe and convert the incoming prop to use noop
so that there is not javascript error?
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.
good idea, will do.
have you used warning
yet anywhere? I had tried to use it before but something was not set up correctly.
What specifically are you referring to by noop
?
I'm assuming you mean react-props-noop
but I don't see that currently listed in our dependencies anywhere.
Or did you mean jQuery.noop()
?
Or something else.
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.
have you used warning yet anywhere? I had tried to use it before but something was not set up correctly.
assets/src/components/form/select/build-options.js)
Also the "coming soon" money and currency vo's I'm working on use warning (I'm still writing tests for them).
What specifically are you referring to by noop ?
lodash.noop
: https://lodash.com/docs/4.17.10#noop
id={ `${ htmlId }-${ description }` } | ||
className={ `${ htmlClass }-${ description } ee-${ description } ee-toggle-button` } | ||
onClick={ ( event ) => toggleCallback( event ) } | ||
onKeyPress={ ( event ) => toggleCallback( event ) } |
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 think you should use onKeyDown
here instead. onKeyPress
ignores certain keys (shift/ctrl etc). So if someone wants to bind those keys to this button they won't be able to.
Also, I'm wary about you calling the same callback for the onClick
and onKeyPress
events here. That could lead to some unexpected behaviour for the implementor. Instead you should look for specific onClick
and onKeyPress
(or if you change it, onKeyDown
) props. Don't create a new named prop for something that is already familiar. It also makes it very clear what events will get triggered by this component.
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.
onKeyPress ignores certain keys (shift/ctrl etc).
yes because that allows you to listen for key combinations like ALT
+x
instead of firing separately after each onKeyDown
event.
So conversely, using onKeyDown
would prevent someone from binding key combinations from being used, so changing to onKeyDown
has its detractors as well right? Question is, how often do you press (shift/ctrl etc) on their own and expect anything to happen?
But anyways, the purpose here is to simply trigger the firing of the callback any time the button is clicked, or any time a key is pressed when the button has keyboard focus, which is satisfied by using a combination of onClick
and onKeyPress
.
Plz keep in mind that not everything we build has to be approached from the perspective of being some completely abstracted library that is intended for release to the general public and therefore needs to satisfy every possible use case and scenario. We are not WordPress. It is perfectly acceptable for us to develop some tools that are only intended for our own purposes at the moment, and IF use by third parties becomes apparent, then those tools can be improved if need be. For now, I am only adding these components so that I can achieve other goals (REM). Our primary objective is not to build a library of generic tools for release on npm.
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.
Plz keep in mind that not everything we build has to be approached from the perspective of being some completely abstracted library that is intended for release to the general public and therefore needs to satisfy every possible use case and scenario.
Then don't add this to our @eventespresso/components
library which is intended for re-use. Just do what you need for REM in REM's own component library and once you've determined the appropriate form for re-usability then we can add something to @eventespresso/components.
I am going to be viewing/evaluating things added to @eventespresso/components
from the standpoint of reusability not only within EE core but also across other add-ons.
So, if your answers for my line of queries are, "I'm only doing this because REM needs it this way" and thus it has a very opinionated behaviour, just keep the component in the add-on you are using it in. However if you're willing to consider how the component might get used in EE core, or other add-ons, then let's iterate further on this in here.
I'm completely fine with some components living in their own add-ons for a while if necessary.
yes it will have to wait for the other two to be approved. such is life sometimes.
because those render actual button elements which would then get styled as buttons whereas this element uses a div and only assigns a button "role" to it, so the final rendered element can be anything, like text or an icon, and will not have additional styles applied to it. I tried to think of a more appropriate name for this component that didn't include the term "button" but still conveyed that concept but could not think if anything. |
Well (I know it doesn't matter) I think there's another way you could do things. You can still create the branch (and even a
If it walks like a duck and quacks like a duck then its a ___? Your answer hasn't really explained why you aren't using an actual button or anchor tag here other than saying you want it to be a div or icon (with role="button"). So let me expand my question, "Why does something that behaves like a button need to be done the way you are doing it here? What's the motivation behind that? What problem is it solving? If your answer involves "which would then get styled as buttons" then I'm not sure why you couldn't just modify the styles of the rendered button or anchor tag? I hope you don't get annoyed with my line of questioning, but I have to ask the questions because although you may understand well the need for this, there's no context in the description of the pull or in any implementation to allow me to understand. |
sry I thought this answer was more than sufficient:
If you want your UI to use an actual HTML
It's not so bad in the WP admin because the main admin "theme" is consistent and can be anticipated. However, for the frontend, it's impossible to know ahead of time what styles might possibly get applied to an element by any of the other stylesheets that get loaded and/or the browser itself, so removing those styles is simply not an option. So if you do not want an element in your UI to be styled like a button, then you don't use a button. For the REM editor I could probably get away with using a component that employs a |
I didn't only mention their button component, I also mentioned their icon button component. As far as I can tell it is a minimally styled component that you can use for similar needs to what you have in this pull: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/icon-button/index.js Another advantage of re-using components that are already available is that WP is taking care to make sure their work is accessibility friendly. I'm aware you used the "role" tag in this pull and maybe that's sufficient. |
killing this in favour of the Gutenberg |
Problem this Pull Request solves
Adds components that can be used to trigger an action like opening or closing a container.
ToggleButtonWithHoverText is wrapped with new HoverText component added in #597.
How has this been tested
includes unit tests and currently in use in the REM add-on
Checklist
esc_html__()
, see https://codex.wordpress.org/I18n_for_WordPress_Developers)