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
generic component to highlight any part of the ui #7131
generic component to highlight any part of the ui #7131
Conversation
width, | ||
}; | ||
return ( | ||
<Portal> |
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.
The implementation is exactly same as the Spotlight component from guided tour. Instead of creating a new one you should just move that one to console-shared and add some customization for this use case.
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.
updated with customized spotlight component
6179017
to
cc99d5c
Compare
cc99d5c
to
2848d13
Compare
<div | ||
className={cx({ | ||
'ocs-spotlight__element-highlight-animate': animate, | ||
'ocs-spotlight__element-highlight-noanimate': !animate, | ||
})} | ||
style={style} |
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.
DRY. Store this component in a variable and render conditionally.
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.
updated using variable.
2848d13
to
71cba76
Compare
@nemesis09 Here are the animation specifications from the final design doc: Blink animation is triggered by click/tap initiating step I took a closer look at the GIF and CSS and I think the elements that are missing are the specifications with the arrows next to them. Let me know if you need me to take a closer look at any animation specifications. Here's the link to the final design doc if you need to reference it: https://docs.google.com/document/d/1T41IzREwgfqR6NVpXI8e4psfkBR39gWt0eLfvA9pkGs/edit?usp=sharing |
/assign |
71cba76
to
ddef86c
Compare
updated animation addressing the suggested requirements. PTAL |
The updates you made @nemesis09 look good to me! From the GIFs it looks like the behaviors and timing work out well. |
@mceledonia Here's the PR associated with the animated quick start hints. Feel free to take a look. The GIFs at the top of the PR are the most recent version of the implementation. |
/retest |
@keyframes highlight { | ||
0% { | ||
opacity: 1; | ||
border: var(--pf-global--BorderWidth--xl) solid var(--pf-global--palette--blue-200); |
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.
Why didn't we use box shadow here as shown in https://codesandbox.io/s/highlight-9oizs
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.
the UX for animation was updated
please find the updated UX for animation here
Slack Conversation links:
https://coreos.slack.com/archives/C014NQG3H5L/p1604609753173100
https://coreos.slack.com/archives/C014NQG3H5L/p1604610208176400
@nemesis09 a few observations from your images:
I'm waiting on @mceledonia to provide updates on the animation before reviewing the code. |
I'll run through each step of the animation and add the transitions and timings in code styling. I'll also add some visual aids for each step. For the animation of all hint borders:
The spec for the starting border styling is the following: EDIT NOTE*** @nemesis09 |
Here's an updated implementation of the requested animation: Note that I'm currently working with @mceledonia to tweak the animation (eg border expansion from 4px -> 12px instead of 4px -> 8px) |
@nemesis09 please use the same outline for both the guided tour and quick starts highlight. |
ddef86c
to
c357569
Compare
@@ -0,0 +1,64 @@ | |||
@keyframes expand { |
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.
@keyframes expand { | |
@keyframes co-spotlight-expand { |
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.
done
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.
Shouldn't this be ocs-spotlight-expand
?
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.
yes it should be. my mistake
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.
renamed to ocs-spotlight-expand
} | ||
} | ||
|
||
@keyframes fade-in { |
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.
@keyframes fade-in { | |
@keyframes co-spotlight-fade-in { |
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.
done
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.
@christianvogt @nemesis09 Shouldn't this be ocs-spotlight-expand
since this is in console-shared
package?
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.
Not sure it matters... we have a poor consistency across the console for keyframes and their names having the appropriate module. I think it's because they are only ever used in the one file. I'm indifferent either way though.
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.
yes it should be. my mistake
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.
renamed to ocs-spotlight-expand
} | ||
} | ||
|
||
@keyframes fade-out { |
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.
@keyframes fade-out { | |
@keyframes co-spotlight-fade-out { |
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.
done
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.
@christianvogt @nemesis09 Shouldn't this be ocs-spotlight-expand
since this is in console-shared
package?
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.
yes it should be. my mistake
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.
renamed to ocs-spotlight-expand
animate?: boolean; | ||
hasBackdrop?: boolean; |
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.
/> | ||
); | ||
return ( | ||
<Portal> |
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.
We will need to use popperjs instead so that the spotlight moves as the node underneath it moves.
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.
using popperjs instead of portal
['--ocs-spotlight-top' as React.ReactText]: `${top}px`, | ||
['--ocs-spotlight-left' as React.ReactText]: `${left}px`, | ||
['--ocs-spotlight-height' as React.ReactText]: `${height}px`, | ||
['--ocs-spotlight-width' as React.ReactText]: `${width}px`, |
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.
Clean up dead code.
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.
done
animate?: boolean; | ||
hasBackdrop?: boolean; |
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.
Combine these 2 props into one called interactive
and add a description.
If interactive
it will allow click through and animate. If !interactive
it will display the background and prevent click through.
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.
done
const handleClick = () => setClicked(true); | ||
document.addEventListener('mousedown', handleClick); | ||
return () => { | ||
document.removeEventListener('mousedown', handleClick); | ||
}; |
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 listen for clicks if in interactive
mode.
You should also make sure the click occurs on the target element and not anywhere on screen.
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.
const handleClick = () => setClicked(true); | |
document.addEventListener('mousedown', handleClick); | |
return () => { | |
document.removeEventListener('mousedown', handleClick); | |
}; | |
const handleClick = () => setClicked(true); | |
element.addEventListener('mousedown', handleClick); | |
return () => { | |
element.removeEventListener('mousedown', handleClick); | |
}; |
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.
the goal for this function is to detect any click outside the link
and stop the animation.
The solid rectangle hint persists until the user clicks somewhere else on the UI or waits the 10 seconds until it fades away.
although reading the uxd again, it seems the click-anywhere-to-disappear
should only apply to the solid rectangle persisting after the expand animation. I'll check this with ux
pointer-events: none; | ||
position: absolute; | ||
&__with-backdrop { | ||
mix-blend-mode: hard-light; |
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 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.
raised issue https://issues.redhat.com/browse/ODC-5178
c357569
to
1e004d0
Compare
document.addEventListener('mousedown', handleClick); | ||
return () => { | ||
document.removeEventListener('mousedown', handleClick); |
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.
document.addEventListener('mousedown', handleClick); | |
return () => { | |
document.removeEventListener('mousedown', handleClick); | |
element.addEventListener('mousedown', handleClick); | |
return () => { | |
element.removeEventListener('mousedown', handleClick); |
This was not updated...
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'm not sure if we need to check for click event on the element,
element
is the component to highlight and the UXD says a click anywhere on the screen should stop the animation
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'll check with ux on the design again
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 checked and you are right. They want it to disappear when clicking anywhere.
/retest |
7d2cd99
to
516fe4c
Compare
/lgtm |
/retest |
Odd, pod didn't start...
/retest |
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.
@nemesis09 I was testing this with guided tours by passing interactive prop to spotlight. I noticed some weird behaviors.
- The outside click doesn't work on any part of the document. It only works when clicked on the popper part of the guided tour.
- The borders on the help menu looks off. It looks okay without the
interactive
prop passed but the arrow is still off a bit from the center.
Interesting that the popover for help is also offset. |
@rohitkrai03 are you saying during Guided Tour you are expecting to click on the mask and exit? Because the interactive mode is false and thus the only way to close the modal is to close it via the |
This is odd. As far as I understand, there has been no substantial change in the spotlight as far as the implementation for Guided Tour is concerned. |
@rohitkrai03 / @christianvogt / @nemesis09 Can you fire up any cluster (before this is merged) and check the Guided Tour? Is this a pre-existing issue? I obviously can't reproduce this 😞 |
Update: I tried it out in the cluster, rather than localhost to check the differences. The offset in the arrow seems to be a pre-existing issue. I think this could be related to a similar behaviour in popover arrow that i noticed in the modals for cluster status card items, which later turned out to be a patternfly issue. here is the slack thread https://coreos.slack.com/archives/C6A3NV5J9/p1603962222473200 |
@andrewballantyne I tested guided tour after passing The below screenshot shows the original use of The below screenshot shows the |
Well that's disconcerting... I'll have to test this out once I am finished with my current task.
This is very confusing lol... I can't reproduce this in any fashion... I will have to try all the browsers under my disposal to see if I can reproduce this. Either way, I think we are addressing this post this PR merge. |
}; | ||
|
||
const Spotlight: React.FC<SpotlightProps> = ({ selector, interactive }) => { | ||
const element = document.querySelector(selector); |
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.
const element = document.querySelector(selector); | |
const element = React.useMemo(() => document.querySelector(selector), [selector]); |
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.
done
const isInViewport = (elementToCheck: Element) => { | ||
const rect = elementToCheck.getBoundingClientRect(); | ||
return ( | ||
rect.top >= 0 && | ||
rect.left >= 0 && | ||
rect.bottom <= (window.innerHeight || document.documentElement.clientHeight) && | ||
rect.right <= (window.innerWidth || document.documentElement.clientWidth) | ||
); | ||
}; |
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.
This is a utility function. Move it out of the render closure.
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.
done
const popperOptions: PopperOptions = { | ||
modifiers: { | ||
preventOverflow: { | ||
enabled: 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.
Make this a const outside of the render function.
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.
done
516fe4c
to
383aa54
Compare
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, nemesis09 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Fixes:
https://issues.redhat.com/browse/ODC-4999
Description:
Create a generic component to highlight any part of UI with animation
Screens:
highlight component behaviour ideal
Note: three components are highlighted simultaneously for illustration only.
highlight component behaviour when clicked anywhere on the ui after animation has started
highlight component behaviour autoscroll to bring component to highlight in viewport if it is hidden
guided tour behaviour unchanged with updated
Spotlight
componentTest Coverage:
Browser Conformance: