Skip to content

@kane/prince of hill#6575

Merged
maxbbb merged 36 commits intodevelopfrom
@kane/prince-of-hill
Apr 28, 2025
Merged

@kane/prince of hill#6575
maxbbb merged 36 commits intodevelopfrom
@kane/prince-of-hill

Conversation

@maxbbb
Copy link
Copy Markdown
Contributor

@maxbbb maxbbb commented Apr 16, 2025

What changed (plus any additional context for devs)

Main changes

  • New king of the hill section on the discover screen
  • King of the hill explain sheet when clicking "How it works"
  • Search bar no longer shown on discover screen, new search icon right side of navbar to get to search

Other changes

  • Changed Discover screen background to black in dark mode
  • Refactored GradientText component a little, cleaned up where it was used
  • Removed openVerfiedExplainer logic in search section list, as the logic for determining if the section was the verified seciton was wrong so it never worked anyways. Also, unlikely users even know you can click the header
  • Fixed duplicate separator from empty RemoteCardCarousel
  • Removed unnecessary state update in a useEffect in the PointsProfileContext
  • Minor Discover search screen cleanups

Screen recordings / screenshots

RPReplay_Final1745539988.mov

What to test

  • Have to turn on the "King of the Hill" feature flag in the dev settings
  • Backend currently returns same mock data every time, so can't truly test

@linear
Copy link
Copy Markdown

linear bot commented Apr 18, 2025

@maxbbb maxbbb marked this pull request as ready for review April 25, 2025 00:24
@brunobar79
Copy link
Copy Markdown
Contributor

Launch in simulator or device for 9880400

@jinchung jinchung requested review from jinchung and removed request for christianbaroni and natew April 25, 2025 17:26
Copy link
Copy Markdown
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

Seems like there were lots of changes in here unrelated to king of the hill, were those intentional? I also noticed that you left out the Android route for the explain sheet.

const profilesEnabled = useExperimentalFlag(PROFILES);
const marginBottom = TAB_BAR_HEIGHT + safeAreaInsetValues.bottom + 16;
const marginBottom = TAB_BAR_HEIGHT;
// safeAreaInsetValues.bottom + 16;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rm? iirc this was added because search rows were underflowing the bottom tab bar. Might be good to verify this isn't needed anymore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed commented out code but this calc is for the scrollview viewport which with the extra + 16 gets an incorrect calculation that results in a 16 height black bar at the top of the tab bar.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

was GradientText broken here? Or why was this removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The way the previous implementation of GradientText worked would have made it more difficult to adapt here than I think it's worth for the sake of just one explain sheet type.

maxbbb and others added 2 commits April 25, 2025 14:32
@maxbbb
Copy link
Copy Markdown
Contributor Author

maxbbb commented Apr 28, 2025

I fixed the GradientText implementation because it resulted in cutoff text on Android and incorrect spacing on iOS; in general the previous approach was incorrect. A consequence though is that it cannot currently be used with the TextShadow component, so the header title on the explain sheet no longer has the blur effect like it does in the video.

Copy link
Copy Markdown
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

couple nits but looks good now, great work

import { analytics } from '@/analytics';
import { PROFILES, useExperimentalFlag } from '@/config';
import { useAccountSettings, useSearchCurrencyList, usePrevious, useHardwareBackOnFocus } from '@/hooks';
import { useAccountSettings, useSearchCurrencyList, usePrevious, useHardwareBackOnFocus, useDimensions } from '@/hooks';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { useAccountSettings, useSearchCurrencyList, usePrevious, useHardwareBackOnFocus, useDimensions } from '@/hooks';
import { useAccountSettings, useSearchCurrencyList, usePrevious, useHardwareBackOnFocus } from '@/hooks';

unused

const profilesEnabled = useExperimentalFlag(PROFILES);
const marginBottom = TAB_BAR_HEIGHT + safeAreaInsetValues.bottom + 16;
const TOP_OFFSET = safeAreaInsetValues.top + navbarHeight;
const MARGIN_BOTTOM = TAB_BAR_HEIGHT;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick, but if this isn't different than the TAB_BAR_HEIGHT now we should just remove this assignment and use TAB_BAR_HEIGHT directly.

Copy link
Copy Markdown
Member

@jinchung jinchung left a comment

Choose a reason for hiding this comment

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

🌮
tested on iOS

@maxbbb maxbbb merged commit c68d2c8 into develop Apr 28, 2025
6 checks passed
@maxbbb maxbbb deleted the @kane/prince-of-hill branch April 28, 2025 20:16
@brunobar79
Copy link
Copy Markdown
Contributor

Launch in simulator or device for 38390a1

@janicduplessis janicduplessis mentioned this pull request Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants