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.

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

Dapp browser: disable tab closing for empty state #5573

Merged
merged 4 commits into from
Mar 30, 2024

Conversation

benisgold
Copy link
Member

Fixes APP-####

What changed (plus any additional context for devs)

  • should not be able to close tab if there's only one open and it's the homepage

Screen recordings / screenshots

What to test

const opacity = tabViewVisible?.value || !isActiveTab ? interpolatedOpacity : withTiming(0, TIMING_CONFIGS.fastFadeConfig);

const opacity =
(multipleTabsOpen || !isOnHomepage) && (tabViewVisible?.value || !isActiveTab)
Copy link
Member Author

Choose a reason for hiding this comment

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

might need diff animation config here once we add animation for going from multiple tabs -> single tab

Copy link
Member

@christianbaroni christianbaroni left a comment

Choose a reason for hiding this comment

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

nice, in the swipeToCloseTabGestureHandler though I'd remove all of the early returns you added and instead modify onEnd like this, so that it rebounds instead of blocking the gesture:

onEnd: (e, ctx: { startX?: number }) => {
  if (!tabViewVisible?.value) return;

  const xDelta = e.absoluteX - (ctx.startX || 0);
  setNativeProps(scrollViewRef, { scrollEnabled: !!tabViewVisible?.value });

  const isBeyondDismissThreshold = (xDelta < -(TAB_VIEW_COLUMN_WIDTH / 2 + 20) && e.velocityX <= 0);
  const isFastLeftwardSwipe = e.velocityX < -500;

  if ((isBeyondDismissThreshold || isFastLeftwardSwipe) && !isEmptyState) {
    const xDestination = -Math.min(Math.max(deviceWidth * 1.25, Math.abs(e.velocityX * 0.3)), 1000);
    gestureX.value = withTiming(xDestination, TIMING_CONFIGS.tabPressConfig, () => {
      runOnJS(closeTab)(tabId);
      gestureScale.value = 0;
      gestureX.value = 0;
      gestureY.value = 0;
      ctx.startX = undefined;
    });
  } else {
    gestureScale.value = withTiming(1, TIMING_CONFIGS.tabPressConfig);
    gestureX.value = withTiming(0, TIMING_CONFIGS.tabPressConfig);
    gestureY.value = withTiming(0, TIMING_CONFIGS.tabPressConfig);
    ctx.startX = undefined;
  }
},

Copy link
Member

@christianbaroni christianbaroni left a comment

Choose a reason for hiding this comment

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

looks good aside from my previous comment

@@ -616,13 +617,13 @@ export const BrowserTab = React.memo(function BrowserTab({ tabId, tabIndex, inje

const swipeToCloseTabGestureHandler = useAnimatedGestureHandler<PanGestureHandlerGestureEvent>({
onStart: (_, ctx: { startX?: number }) => {
if (!tabViewVisible?.value) return;
Copy link
Member

@christianbaroni christianbaroni Mar 29, 2024

Choose a reason for hiding this comment

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

these early returns should be removed once we fix the disabling of the gesture handlers outside of the tab view

probably currently allows the tab to get stuck if while dragging a tab you exit the tab view

we could even remove the early return in onEnd now and add a !!tabViewVisible?.value condition where I added !isEmptyTab:

onEnd: (e, ctx: { startX?: number }) => {
  const xDelta = e.absoluteX - (ctx.startX || 0);
  setNativeProps(scrollViewRef, { scrollEnabled: !!tabViewVisible?.value });

  const isBeyondDismissThreshold = (xDelta < -(TAB_VIEW_COLUMN_WIDTH / 2 + 20) && e.velocityX <= 0);
  const isFastLeftwardSwipe = e.velocityX < -500;
  const shouldDismiss = !!tabViewVisible?.value && !isEmptyState && (isBeyondDismissThreshold || isFastLeftwardSwipe);

  if (shouldDismiss) {
      
    // rest the same

@christianbaroni
Copy link
Member

✅✅

@christianbaroni christianbaroni merged commit 407aed2 into develop Mar 30, 2024
5 of 6 checks passed
@christianbaroni christianbaroni deleted the @benisgold/closetab branch March 30, 2024 06:32
BrodyHughes added a commit that referenced this pull request Apr 2, 2024
* 'develop' of github.com:rainbow-me/rainbow:
  Only hold the active tab ref in BrowserContext (#5579)
  Dapp browser: disable tab closing for empty state (#5573)
  Browser: fix ref assignment, back/forward navigation (#5578)
  Browser: fully eliminate reloading issues (#5576)
  browser: static trending dapps (#5561)
  bump swaps sdk (#5574)
  fix gitignore (#5571)
  bump (#5570)
  Fix browser context menu not updating (#5569)
  ⚡️ Fast browser (#5566)
  [APP-1049]: (feat): Backups V2 (#5310)
  fix: search by contract address (#5563)
BrodyHughes added a commit that referenced this pull request Apr 10, 2024
…e-changes

* 'develop' of github.com:rainbow-me/rainbow: (44 commits)
  allow open in new tab (#5610)
  added warning for unknown price impact (#5597)
  fix cloudflare protection (#5609)
  improve type checking on web preferences (#5607)
  fix scrolltoindex firing on last card dismissal (#5606)
  make account network switcher work (#5604)
  Dapp browser fixes (#5596)
  Fix close tab btn (#5598)
  browser: add account context menu (#5603)
  Fix instant screenshot setting (#5602)
  swaps: bump sdk (#5583)
  Browser: tab transitions, state update queue (#5582)
  audit: undici (#5594)
  [SWAPS V2]: Add token search logic and ability to select assets (#5547)
  swaps v2 gas (#5526)
  tx sim: other natives (#5585)
  Brody/bump 1.9.21 3 (#5588)
  fix sheet bg (#5590)
  Only hold the active tab ref in BrowserContext (#5579)
  Dapp browser: disable tab closing for empty state (#5573)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants