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.

Sign up for GitHub

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

T352298: Add custom tooltip #111

Merged
merged 24 commits into from
Apr 29, 2024
Merged

T352298: Add custom tooltip #111

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0239564
Add initial tooltip.js
medied Mar 27, 2024
2e50f7a
Initial computeTooltipPosition
medied Mar 28, 2024
9f475a2
Use Popover component
medied Apr 2, 2024
4e44ddd
Naming update
medied Apr 3, 2024
e24f898
Address warnings
medied Apr 3, 2024
da477e3
Move useEffect logic to a method for consistency
medied Apr 3, 2024
2c8c1f4
Use property to store tooltip count
medied Apr 8, 2024
089406b
Remove className
medied Apr 8, 2024
1c64954
Add guard to prevent displaying tooltip if toolbar is closed
medied Apr 8, 2024
cf59256
Address php linter
medied Apr 9, 2024
c9c1363
Proprogate property
medied Apr 9, 2024
03f74a5
Color popover arrow and add offset
medied Apr 10, 2024
5f28125
Rewrite endpoints set up as a function for consistency
medied Apr 10, 2024
a66986f
Address linter
medied Apr 10, 2024
62b3868
Increment property via window object
medied Apr 10, 2024
f8f3546
Merge branch 'T352298-tooltip-property' into T352298-tooltip
medied Apr 10, 2024
995ab80
Use camelCasing and improve readability
medied Apr 10, 2024
51680af
Concentrate all tooltip logic inside CustomTooltip for better code hy…
medied Apr 12, 2024
8e8b8bc
Disable selector-class-pattern globally
medied Apr 12, 2024
f83aa6d
Track whether the tooltip has been displayed for the full 5 secs
medied Apr 18, 2024
20ed74b
Address linter
medied Apr 18, 2024
afa3625
Clear timeouts if user clicks on W before the tooltip diplays
medied Apr 24, 2024
1661af8
Address linter
medied Apr 24, 2024
0e297c4
Add comments for php properties
medied Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions src/link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useState } from '@wordpress/element';
import { useEffect, useState, useRef } from '@wordpress/element';
import { BlockControls } from '@wordpress/block-editor';
import { ToolbarGroup, ToolbarButton } from '@wordpress/components';
import {
Expand All @@ -10,6 +10,7 @@
import { __ } from '@wordpress/i18n';
import { InlineEditUI } from './inline';
import { PreviewEditUI } from './preview';
import { CustomTooltip } from './tooltip';

const formatType = 'wikipediapreview/link';
const formatTitle = __( 'Wikipedia Preview', 'wikipedia-preview' );
Expand Down Expand Up @@ -46,6 +47,11 @@
const startViewingPreview = () => setViewingPreview( true );
const stopViewingPreview = () => setViewingPreview( false );
const [ lastValue, setLastValue ] = useState( null );
const toolbarButtonRef = useRef();
const [ displayCustomTooltip, setDisplayCustomTooltip ] = useState( false );
// eslint-disable-next-line no-undef
const customTooltipDisplayedCount = parseInt( localStorage.getItem( 'WikipediaPreviewWordpressPlugin-CustomTooltipDisplayedCount' ) ) || 0;
Copy link
Contributor Author

@medied medied Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opting for localStorage here to save this count state, useState wouldn't work here as it gets reset on page reload. Yet the specification asks to re-display tooltip at least a second time:

Reattempt displaying the tooltip in the subsequent editing session to provide another opportunity for the user to view it. Cap the tooltip reattempts to just one additional session to prevent repetitive intrusion

Hence customTooltipDisplayedLimit which can be increased to 2 (or whatever limit). However this is what I would like to discuss with Sudhanshu, details of this UX, what should we look for here to decide whether to display tooltip again? For example, we could look into whether there are already previews in the post: if no previews added, display again; otherwise we could infer the user learned to add the preview as there are present already.

Also localStorage gets reset on a fresh/new browser session, which makes us run into the risk of the tooltip being annoying. Perhaps another reason to check whether previews are added already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is showing up for editors within the admin interface, we can use a property for storage. Like we did for the review banner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, here's one implementation: 2c8c1f4, do you think there's a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exploring with a propagated property here but looks like the global var only gets updated on page reload so it may not work for what we need

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I miss the context here, this need to be documented well and easy to confuse if we revisit this logic again in few months/years later.

If the toolbar is closed before the 5-second display period, skip the tooltip for the remainder of the session.

This can handled by localStorage, so it checks through the current session

Once the tooltip has been displayed for the full duration, do not show it in future sessions to avoid redundancy.

full duration for 5 seconds, then we set post the incrementDisplayedCount

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm exploring with a propagated property here but looks like the global var only gets updated on page reload so it may not work for what we need

It's propagated from php to js on page load. After that, you have to update it yourself on the js side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's propagated from php to js on page load. After that, you have to update it yourself on the js side.

Got it, updating directly on JS side by incrementing directly via window (and without useState): 62b3868

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I miss the context here, this need to be documented well...

Yea this is part of the UX I would like to clarify with Sudhanshu, specially the other point:

Reattempt displaying the tooltip in the subsequent editing session to provide another opportunity for the user to view it. Cap the tooltip reattempts to just one additional session to prevent repetitive intrusion

I think we just need to first make sure we're on the same page of what a 'session' is in this case, and also just agree on when exactly we should display tooltip again. Let's circle back to this later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realise there is a sessionStorage https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage in browser, if we are dealing with session maybe this is what we can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I came across that method too, but it's not really suited as we need the storage to be more permanent. From the docs:

...data in sessionStorage is cleared when the page session ends.

What we need is to be able to display the tooltip with the full 5 seconds duration, and then we don't show again.

const customTooltipDisplayedLimit = 1;

const formatButtonClick = () => {
if ( isActive ) {
Expand Down Expand Up @@ -175,6 +181,14 @@
}
};

const waitOneSecThenDisplayCustomTooltip = () => {
setTimeout( () => {
setDisplayCustomTooltip( true );
// eslint-disable-next-line no-undef
localStorage.setItem( 'WikipediaPreviewWordpressPlugin-CustomTooltipDisplayedCount', customTooltipDisplayedCount + 1 );
}, 1000 );
hueitan marked this conversation as resolved.
Show resolved Hide resolved
};

useEffect( () => {
if ( Object.keys( activeAttributes ).length ) {
stopAddingPreview();
Expand All @@ -191,20 +205,35 @@
handleTextEdit();
setLastValue( value );
}
}, [ value ] );

Check warning on line 208 in src/link/edit.js

View workflow job for this annotation

GitHub Actions / build 

React Hook useEffect has missing dependencies: 'handleTextEdit' and 'lastValue'. Either include them or remove the dependency array

useEffect( () => {
if ( customTooltipDisplayedCount < customTooltipDisplayedLimit ) {
// Wait 1 second and then display tooltip
waitOneSecThenDisplayCustomTooltip();
}
} );

return (
<>
<BlockControls>
<ToolbarGroup>
<ToolbarButton
icon={ generateWikipediaLogo( isActive ? 'white' : 'black' ) }
title={ formatTitle }
title={ __( 'Add Wikipedia Preview', 'wikipedia-preview' ) }
isActive={ isActive }
onClick={ formatButtonClick }
className="wikipediapreview-edit-toolbar-button"
medied marked this conversation as resolved.
Show resolved Hide resolved
ref={ toolbarButtonRef }
/>
</ToolbarGroup>
</BlockControls>
{ displayCustomTooltip && (
<CustomTooltip
anchorRef={ toolbarButtonRef }
setDisplayCustomTooltip={ setDisplayCustomTooltip }
/>
) }
{ addingPreview && (
<InlineEditUI
contentRef={ contentRef }
Expand Down
13 changes: 13 additions & 0 deletions src/link/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,19 @@ body {
animation-name: wikipediapreview-edit-inline-loader-expanded;
}
}

&-tooltip {
width: 190px;
height: 40px;
background: #36c;
color: #fff;
display: flex;
justify-content: center;

&-text {
align-self: center;
}
}
hueitan marked this conversation as resolved.
Show resolved Hide resolved
hueitan marked this conversation as resolved.
Show resolved Hide resolved
}

.wikipediapreview-edit-preview-popover {
Expand Down
29 changes: 29 additions & 0 deletions src/link/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { __ } from '@wordpress/i18n';
import { Popover } from '@wordpress/components';
import { useEffect } from '@wordpress/element';

export const CustomTooltip = ( {
anchorRef,
setDisplayCustomTooltip,
} ) => {
useEffect( () => {
// Wait 5 seconds and then hide the tooltip
setTimeout( () => {
setDisplayCustomTooltip( false );
}, 5000 );
} );

return (
<div>
<Popover
anchor={ anchorRef.current }
placement={ 'top' }
noArrow={ false }
>
<div className="wikipediapreview-edit-inline-tooltip">
<p className="wikipediapreview-edit-inline-tooltip-text">{ __( 'Add Wikipedia Preview' ) }</p>
</div>
</Popover>
</div>
);
};
Loading