feat: Feature prompt internal component #4170
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4170 +/- ##
=======================================
Coverage 97.15% 97.15%
=======================================
Files 875 878 +3
Lines 25673 25716 +43
Branches 9293 9297 +4
=======================================
+ Hits 24942 24985 +43
- Misses 684 725 +41
+ Partials 47 6 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
georgylobko
force-pushed
the
feat/feature-prompt
branch
from
3d1598f to
049f0d9
Compare
georgylobko
force-pushed
the
feat/feature-prompt
branch
from
049f0d9 to
93ff28b
Compare
georgylobko
force-pushed
the
feat/feature-prompt
branch
from
f373397 to
072547d
Compare
georgylobko
requested review from
avinashbot and
pan-kot
and removed request for
a team
pages/app/index.tsx
Outdated
| 'charts.test', | ||
| 'error-boundary/demo-async-load', | ||
| 'error-boundary/demo-components', | ||
| 'features-notifications', |
There was a problem hiding this comment.
nit: should the page name be "feature-notifications"?
| </Box> | ||
| } | ||
| getTrack={() => document.querySelector('#settings-icon')} | ||
| trackKey="settings-icon" |
There was a problem hiding this comment.
nit: is the trackKey really needed here? For statically positioned prompts I don't think it is important.
| /> | ||
| <AppLayout | ||
| ariaLabels={labels} | ||
| analyticsMetadata={{ |
There was a problem hiding this comment.
nit: do we need metadata props on this page?
src/internal/do-not-use/feature-prompt/__tests__/feature-prompt.test.tsx
Show resolved
Hide resolved
| { size = 'medium', position = 'top', ...rest }: FeaturePromptProps, | ||
| ref: React.Ref<FeaturePromptProps.Ref> | ||
| ): JSX.Element => { | ||
| const baseComponentProps = useBaseComponent('FeaturePromptProps', { props: { size, position } }); |
There was a problem hiding this comment.
| const baseComponentProps = useBaseComponent('FeaturePromptProps', { props: { size, position } }); | |
| const baseComponentProps = useBaseComponent('FeaturePrompt', { props: { size, position } }); |
| })); | ||
|
|
||
| return ( | ||
| <span {...baseProps} className={clsx(styles.root)} ref={__internalRootRef}> |
| }} | ||
| variant="annotation" | ||
| overflowVisible="content" | ||
| onBlur={() => { |
There was a problem hiding this comment.
Won't the onBlur also fire if the focus moves between focusable elements inside the feature prompt (e.g. when moving focus from the dismiss button to a link in the content)?
There was a problem hiding this comment.
Valid point. Yes, addressed
| Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
There was a problem hiding this comment.
No, they aren't needed. Removed
| const baseProps = getBaseProps(restProps); | ||
| const [show, setShow] = useState(false); | ||
|
|
||
| const popoverBodyRef = useRef<HTMLDivElement | null>(null); |
There was a problem hiding this comment.
for onBlur handling. btw, I have introduced this handling before, but didn't push here
georgylobko
force-pushed
the
feat/feature-prompt
branch
from
45d1680 to
028b5a5
Compare
| overflowVisible="content" | ||
| onBlur={event => { | ||
| const relatedTarget = event.relatedTarget; | ||
| if (relatedTarget && popoverBodyRef.current?.contains(relatedTarget)) { |
There was a problem hiding this comment.
I think there is no need to pass the ref to the popover body - the same can be achieved by adding a div wrapper above the container and attach the ref there.
There was a problem hiding this comment.
What is the difference between my approach and your suggestion?
There was a problem hiding this comment.
There is no functional diff - it just helps to keep the popover body api slightly simpler. It is up to you yo decide
There was a problem hiding this comment.
The same applies to onBlur - it can technically be done outside, too.
2 participants
Description
Introduced a new internal component FeaturePrompt as part of the feature notification pattern.
Related links, issue #, if available: n/a
How has this been tested?
U tests
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.