-
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 HoverText UI Component #597
Closed
Closed
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
d8162dc
HoverText functional component
tn3rb ad545bd
HoverText stylesheet
tn3rb b14f2ee
export ui components
tn3rb 84e41b1
HoverText unit tests
tn3rb 2f2b2ee
wrap the actual hover text in a span
tn3rb 51d679a
update tests after component change
tn3rb 17e3b97
update prop order and doc text
tn3rb bf6a09d
HoverText documentation
tn3rb 4263fe8
ui readme entry
tn3rb 45fece2
Merge branch 'Gutenberg/master' into Gutenberg/components-ui-hover-text
tn3rb 272a8f6
add eejs.hocComponents as external for components
tn3rb 08573f0
add rules for css loading to components config
tn3rb 6a2b450
add CSS_HANDLE_EE_COMPONENTS const and register stylesheet
tn3rb 4952ad0
Merge branch 'Gutenberg/enable-component-css' into Gutenberg/componen…
tn3rb 5568531
rebuild assets
tn3rb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
104 changes: 99 additions & 5 deletions
104
...e-components.70edb8ff0de64cc4b3a2.dist.js → ...e-components.31274ce12670a8583471.dist.js
Large diffs are not rendered by default.
Oops, something went wrong.
111 changes: 111 additions & 0 deletions
111
assets/dist/ee-components.e901a343f37109e97193.dist.css
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import './style.css'; | ||
|
|
||
| export const HOVER_TEXT_POSITION_TOP_LEFT = 'top-left'; | ||
| export const HOVER_TEXT_POSITION_TOP_RIGHT = 'top-right'; | ||
| export const HOVER_TEXT_POSITION_BOTTOM_LEFT = 'bottom-left'; | ||
| export const HOVER_TEXT_POSITION_BOTTOM_RIGHT = 'bottom-right'; | ||
|
|
||
| /** | ||
| * HoverText | ||
| * Adds a text element that is only displayed when the wrapped component (children) is hovered over | ||
| * | ||
| * @function | ||
| * @param {string} hoverText The actual text to be displayed when the child entity is hovered over | ||
| * @param {string} htmlId Used to generate an HTML "id" attribute. | ||
| * The description and "-hover-text" are appended. | ||
| * @param {string} htmlClass Used to generate an HTML "class" attribute. | ||
| * The description and "-hover-text" are appended. | ||
| * @param {string} description Two or three words (hyphenated) that describe the component | ||
| * that the hover text is being added to | ||
| * @param {string} position Where the hover text appears relative to the wrapped component | ||
| * @param {string} children The child entity that gets wrapped with the hover elements | ||
| * @return {string} The child entity wrapped with the hover elements | ||
| */ | ||
| export const HoverText = ( { hoverText, htmlId, htmlClass, description, position, children } ) => { | ||
| position = position ? | ||
| position : | ||
| HOVER_TEXT_POSITION_TOP_RIGHT; | ||
| const pointerCharCodes = {}; | ||
| pointerCharCodes[ HOVER_TEXT_POSITION_TOP_LEFT ] = 9701; | ||
| pointerCharCodes[ HOVER_TEXT_POSITION_TOP_RIGHT ] = 9700; | ||
| pointerCharCodes[ HOVER_TEXT_POSITION_BOTTOM_LEFT ] = 9698; | ||
| pointerCharCodes[ HOVER_TEXT_POSITION_BOTTOM_RIGHT ] = 9699; | ||
| return hoverText && ( | ||
| <div | ||
| id={ `${ htmlId }-${ description }-hover-text` } | ||
| className={ `${ htmlClass }-${ description }-hover-text ee-hover-text-position-${ position } ee-${ description } ee-hover-text` } | ||
| > | ||
| <div className="ee-hover-text-content"> | ||
| { children } | ||
| </div> | ||
| <div className={ 'ee-hover-text-notice-wrapper' }> | ||
| <div className="ee-hover-text-notice ee-small-shadow"> | ||
| <span className={ 'ee-hover-text-text' }> | ||
| { hoverText } | ||
| </span> | ||
| <span className={ 'ee-hover-text-pointer ee-small-text-shadow' }> | ||
| { String.fromCharCode( pointerCharCodes[ position ] ) } | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /* ------------------------------------------------------------------------- | ||
| * Event Espresso HoverText Stylesheet | ||
| * (c) 2018 Event Espresso | ||
| * -------------------------------------------------------------------------*/ | ||
|
|
||
| .ee-hover-text { | ||
| cursor: pointer; | ||
| display: inline-block; | ||
| position: relative; | ||
| } | ||
|
|
||
| .ee-hover-text-notice-wrapper { | ||
| left: 50%; | ||
| overflow: visible; | ||
| position: absolute; | ||
| top: 0; | ||
| z-index: 999999; | ||
| } | ||
|
|
||
| .ee-hover-text .ee-hover-text-notice { | ||
| background-color: #58575c; | ||
| color: #fff; | ||
| display: inline-block; | ||
| font-size: 12px !important; | ||
| opacity: 0; | ||
| padding: .5rem 1rem; | ||
| position: absolute; | ||
| visibility: hidden; | ||
| white-space: nowrap; | ||
| transition: opacity 250ms 125ms, visibility 250ms 125ms; | ||
| } | ||
|
|
||
| .ee-hover-text-pointer { | ||
| color: #58575c; | ||
| text-shadow: 0 2px 4px rgba(0, 0, 0, .5); | ||
| display: inline-block; | ||
| position: absolute; | ||
| } | ||
|
|
||
| .ee-hover-text:hover .ee-hover-text-notice { | ||
| opacity: 1; | ||
| visibility: visible; | ||
| transition: opacity 250ms 500ms, visibility 250ms 500ms; | ||
| } | ||
|
|
||
| /* HOVER_TEXT_POSITION_TOP_LEFT */ | ||
| .ee-hover-text-position-top-left .ee-hover-text-notice { | ||
| right: -.7rem; | ||
| top: -3.2rem; | ||
| } | ||
|
|
||
| .ee-hover-text-position-top-left .ee-hover-text-pointer { | ||
| right: 1rem; | ||
| bottom: -.93rem; | ||
| } | ||
|
|
||
| .ee-toggle-modal-close.ee-hover-text-position-top-left .ee-hover-text-notice { | ||
| right: .9rem; | ||
| top: -2.4rem; | ||
| } | ||
|
|
||
| /* HOVER_TEXT_POSITION_TOP_RIGHT */ | ||
| .ee-hover-text-position-top-right .ee-hover-text-notice { | ||
| left: -.6rem; | ||
| top: -3.2rem; | ||
| } | ||
|
|
||
| .ee-hover-text-position-top-right .ee-hover-text-pointer { | ||
| left: 1rem; | ||
| bottom: -.93rem; | ||
| } | ||
|
|
||
| .ee-toggle-modal-close.ee-hover-text-position-top-right .ee-hover-text-notice { | ||
| left: -2.2rem; | ||
| top: -2.4rem; | ||
| } | ||
|
|
||
| /* HOVER_TEXT_POSITION_BOTTOM_LEFT */ | ||
| .ee-hover-text-position-bottom-left .ee-hover-text-notice { | ||
| right: -.6rem; | ||
| top: 2.5rem; | ||
| } | ||
|
|
||
| .ee-hover-text-position-bottom-left .ee-hover-text-pointer { | ||
| right: 1rem; | ||
| top: -.93rem; | ||
| } | ||
|
|
||
| .ee-toggle-modal-close.ee-hover-text-position-bottom-left .ee-hover-text-notice { | ||
| right: 1rem; | ||
| top: 3.2rem; | ||
| } | ||
|
|
||
| /* HOVER_TEXT_POSITION_BOTTOM_RIGHT */ | ||
| .ee-hover-text-position-bottom-right .ee-hover-text-notice { | ||
| left: -.6rem; | ||
| top: 2.5rem; | ||
| } | ||
|
|
||
| .ee-hover-text-position-bottom-right .ee-hover-text-pointer { | ||
| left: 1rem; | ||
| top: -.93rem; | ||
| } | ||
|
|
||
| .ee-toggle-modal-close.ee-hover-text-position-bottom-right .ee-hover-text-notice { | ||
| left: -2.2rem; | ||
| top: 3.2rem; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { shallow } from 'enzyme'; | ||
| import { HoverText } from "../"; | ||
|
|
||
|
|
||
| describe( 'HoverText', () => { | ||
| it( 'renders nothing if hoverText not supplied', () => { | ||
| const wrapper = shallow( <HoverText /> ); | ||
| expect( wrapper.find( '.ee-hover-text' ) ).toHaveLength( 0 ); | ||
| } ); | ||
| it( 'renders default when hoverText supplied', () => { | ||
| const wrapper = shallow( <HoverText hoverText={ 'click-this' } /> ); | ||
| expect( wrapper ).toHaveLength( 1 ); | ||
| const hover = wrapper.find( '.ee-hover-text' ); | ||
| expect( hover ).toHaveLength( 1 ); | ||
| expect( hover.props().id ).toEqual( 'undefined-undefined-hover-text' ); | ||
| expect( hover.props().className ).toEqual( | ||
| 'undefined-undefined-hover-text ee-hover-text-position-top-right ee-undefined ee-hover-text' | ||
| ); | ||
| } ); | ||
| it( 'renders correctly when all props supplied', () => { | ||
| const wrapper = shallow( | ||
| <HoverText | ||
| hoverText={ 'click-this' } | ||
| htmlId={ 'test-id' } | ||
| htmlClass={ 'test-class' } | ||
| description={ 'what-to-do' } | ||
| dashicon={ 'yes' } | ||
| > | ||
| <div>CLICK ME</div> | ||
| </HoverText> | ||
| ); | ||
| expect( wrapper ).toHaveLength( 1 ); | ||
| const hover = wrapper.find( '.ee-hover-text' ); | ||
| expect( hover ).toHaveLength( 1 ); | ||
| expect( hover.props().id ).toEqual( 'test-id-what-to-do-hover-text' ); | ||
| expect( hover.props().className ).toEqual( | ||
| 'test-class-what-to-do-hover-text ee-hover-text-position-top-right ee-what-to-do ee-hover-text' | ||
| ); | ||
| const text = wrapper.find( '.ee-hover-text-text' ); | ||
| expect( text ).toHaveLength( 1 ); | ||
| expect( text.text() ).toEqual( 'click-this' ); | ||
| const content = wrapper.find( '.ee-hover-text-content' ); | ||
| expect( content ).toHaveLength( 1 ); | ||
| expect( content.text() ).toEqual( 'CLICK ME' ); | ||
| } ); | ||
| } ); | ||
|
|
||
| // assets/src/components/ui/hover-text/test/index.js |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| export { HoverText } from './hover-text'; | ||
| export * from './image'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
There's potential for non unique ids to be implemented if there are multiple instances of this component in use. Granted it would mean that the developer would have to provide the same
htmlIdanddescriptionfor this to happen but its still possible. I think it'd be good to wrap this with thewithInstanceIdHOC so the id is guaranteed to be 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.
I can do that, but it's not actually critical that this has a totally unique id as we are not using that for any kind of targeting. It's really only needed if a dev wants to tweak the styling for a specific hover text in a component, in which case, the dev can simply ensure that it has a unique id, by passing one.
The
withInstanceIdHOC was necessary in Gutenberg because of the way blocks and the GB admin works which absolutely requires unique ids in order to function properly, otherwise performing an action like deleting a block could affect more than one instance. For our purposes we are not using ids for control like that, and would only ever need it for styling purposes (remember this is just hover text) so having an id that potentially changes due to the number of calls towithInstanceIdwould actually be counter to our purposes.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.
So its okay to have potentially invalid html rendered?
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.
If an html id attribute is not needed then I would say don't bother adding it.
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.
What purposes would it be counter to?
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.
If that's the case, and you decide to keep this component, I'd prefer it if you either:
htmlClassprop. I don't see how having an id is necessary if they can provide a className. If an id is necessary for styling then that is argument for it being unique.OR
withInstanceIdand implement so the html id attribute is unique.However, my preference is that you just use
PopOvercomponent from@wordpress/componentsbecause from what I've seen so far, I don't see the need for re-inventing the wheel here.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.
No you are making a strawman argument as that was not my point. A form input for example can still work correctly even if a developer fails to give it a unique id. That does not mean that invalid html is ok, it only means that nothing breaks as a result of that invalid html. With Gutenberg blocks, it is critical that some components have a unique id else the editor will not be able to target them properly and things will not work.
class names are not intended to be unique so if you have multiple elements that share the same class, then you need to start relying on more complex css to target a specific element, which may not always be possible.
There is nothing preventing the
htmlIdprop here from being unique. It CAN be unique simply by setting a unique value when adding the component to your app. This is no different than our PHP form system that generates ids for form elements in that it is up to the developer to ensure that the data they are inputting results in valid html. If i had two inputs in a row in a form and used "my-input" for the id for both inputs, then that would result in invalid html but it would NOT prevent the form from operating correctly in any other way, as long as I did not require those input ids for any kind of targeting (JS or CSS). Having a duplicate id for some hover text is not good, but it will not break anything.With Gutenberg blocks it is a totally different scenario because the editor can create multiple instances of a block dynamically, so I could add one, two, three... ten... one hundred... (one miiiillllliiiion) copies of the same block to a post, and in order for GB to be able to target each one of those instances separately, they all need to have an id that is at least partially dynamically generated. This can also apply to components being used within the block, especially if you are adding form elements to the Gutenberg sidebar, because again, those may need to have a unique id for targeting purposes.
Here is the first sentence from the documentation for the
withInstanceIdHOC:There is nothing in the documentation stating that every single component that implements an id must absolutely use the
withInstanceIdHOC in order to guarantee the uniqueness of that id. But SOME components do need to generate a unique id for each instance. To suggest that a non-critical component like some hover text needs to utilize thewithInstanceIdHOC in order to guarantee a totally unique id attribute is totally overboard. If it is that much of an issue, then the more sane approach would be to use thewithInstanceIdHOC on a more appropriate parent component higher up in the hierarchy of your app, and then pass that id down to any child elements and have them incorporate that into any ids that they set. In other words, there is no reason why the html id for some hover text could not incorporate a unique id that was passed down from awithInstanceIdHOC that was used higher up in the app.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 understand how
withInstanceIdworks and its purpose. I'm not making a straw man argument. My point is that IF someone was to generate a bunch of these components without passing in a uniquehtmlIdprop (and other props used for the id are the same), then the result would be invalid html rendered. I'm not saying it won't work, I'm saying it would be invalid. So my question was simply asking "So its okay to have potentially invalid html rendered?" (note I emphasized potentially here). Your answer can simply be that you believe it is.I still stand by my outlined preferences here.
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.
There is nothing stopping this html id prop from being unique, but the dev implementing it in their component would be responsible for actually generating a unique id in the first place, and that is no different than a bunch of our other systems that we already have in place (like our form system).
Again, the sane thing to do would be to use
withInstanceIdon a more appropriate parent component in any app that can be implemented more than once on the same page, and then pass that unique id down through the component hierarchy. In other words, only use thewithInstanceId HOCfor things that can have multiple instances generated dynamically. It's total overkill to employ it every single time you plan to implement an html id attribute.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 understand that. However, that's why one of the options I gave was:
Developers can pass along a unique className for the css class if they want. In other words don't set an id if it's not necessary.
Now you didn't mention it, but just in case its one possible reason you are explicitly adding an id here (beyond for styling purposes), if the id is something you're adding so the element can be targeted by js, it'd be better if you wrap the component with
React.forwardRefand set arefon the element you want targeted as that's the recommended way to integrate custom binding to rendered elements.