chore: cleanup shortcuts experiment #3742
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
| type ShortcutLinksControlProps = { | ||
| className?: string; | ||
| onLinkClick: () => void; | ||
| onOptionsOpen: () => void; | ||
| shortcutLinks: string[]; | ||
| }; | ||
|
|
||
| function ShortcutLinksUIControl(props: ShortcutLinksControlProps) { | ||
| const { shortcutLinks, onLinkClick, onOptionsOpen, className } = props; | ||
| return ( | ||
| <> | ||
| {shortcutLinks?.length ? ( | ||
| <CustomLinks | ||
| links={shortcutLinks} | ||
| className={className} | ||
| onOptions={onOptionsOpen} | ||
| onLinkClick={onLinkClick} | ||
| /> | ||
| ) : ( | ||
| <Button | ||
| className={className} | ||
| variant={ButtonVariant.Tertiary} | ||
| icon={<PlusIcon />} | ||
| iconPosition={ButtonIconPosition.Right} | ||
| onClick={onOptionsOpen} | ||
| > | ||
| Add shortcuts | ||
| </Button> | ||
| )} | ||
| </> |
| {!hideShortcuts && | ||
| (showGetStarted ? ( | ||
| <ShortcutGetStarted | ||
| onTopSitesClick={toggleShowTopSites} | ||
| onCustomLinksClick={onOptionsOpen} | ||
| /> | ||
| ) : ( | ||
| <ShortcutLinksList | ||
| {...{ | ||
| onLinkClick, | ||
| onMenuClick, | ||
| onOptionsOpen, | ||
| shortcutLinks, | ||
| shouldUseListFeedLayout, | ||
| showTopSites, | ||
| toggleShowTopSites, | ||
| hasCheckedPermission, | ||
| }} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
hideShortcuts checks if shortcuts are visible
showGetStarted check if !isOldUser && hasNoShortcuts, basically old users will never see the get started banner even if they don't have any shortcuts atm
| import React, { | ||
| MouseEventHandler, | ||
| PropsWithChildren, | ||
| ReactElement, | ||
| } from 'react'; |
There was a problem hiding this comment.
Removed this component and created new ones for clarity:
- list
- item
- get started
| const { isOldUser, showToggleShortcuts } = useShortcutsUser(); | ||
|
|
||
| const hasNoShortcuts = !shortcutLinks?.length && showTopSites; | ||
| const showGetStarted = !isOldUser && hasNoShortcuts; |
There was a problem hiding this comment.
New logic, check if is old user then hide get started if it is.
@idoshamun yes if I correctly understand this button is visible inside the description's table (I know the preview is very small...). Are you referring to this button, right? 
|
|
I'm referring to this button indeed, just asking if they currently see it or if we introduce it as part of this pr? |
erge branch 'chore-shortcuts-ui' of https://github.com/dailydotdev/apps into chore-shortcuts-ui
@idoshamun I've checked with a non-updated extension and seems nothing have changed for new users ✔️ |
| <ShortcutItemPlaceholder key={url}> | ||
| <img | ||
| src={url} | ||
| alt={url} |
There was a problem hiding this comment.
Not sure the alt should be the url.
There was a problem hiding this comment.
I'm adding some context to alt but unfortunately we don't have any more infos than url at this point
| completeAction(ActionType.FirstShortcutsSession); | ||
| return; |
There was a problem hiding this comment.
After completing the action, it seems that we won't be doing anything here. Is that intended?
There was a problem hiding this comment.
Yes, completing action turn isOldUserWithNoShortcuts into a standard user and enable the shortcuts ✔️
| showGetStarted, | ||
| hideShortcuts: showToggleShortcuts, | ||
| onSaveChanges, | ||
| askTopSitesPermission: async () => { |
There was a problem hiding this comment.
Can't remember if this was used in any dependencies, but should we maybe wrap it in a useCallback to make the returned function stable?
There was a problem hiding this comment.
Well, I've seen most of the functions, are not wrapped, probably good to check whether we should add the wrap.
packages/extension/src/newtab/ShortcutLinks/useShortcutLinks.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Lee Hansel Solevilla <[email protected]>
Original PR from @nensidosari , now mantained by @ilasw
Changes
Summary
Description
Initially this task was about removing the experiment but due to the bad performance of the
Get startedbanner for already existing users (registered before experiment) we added a rule that remove this step for them.Basically this should be every possible option:
Screens
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://chore-shortcuts-ui.preview.app.daily.dev