Skip to content

Hint Refactor #3486

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 42 commits into from
Feb 12, 2025
Merged

Hint Refactor #3486

merged 42 commits into from
Feb 12, 2025

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Jan 21, 2025 •
edited
Loading

Description

Refactor Hint component to make it more readable and maintainable

  • I changed implementation for the hint positioning so it cover more random edge cases
  • I broke into smaller components
  • I add separated logic to hooks
  • I reduced a lot of code redundancy
  • Implemented a new driver and added minimal tests (there are some limitations with testing the Hint component)

How to review
I started with breaking this refactor into several different PRs, but TBH I think it's best to just review it as new code from scratch so I submitted only the last PR

Changelog

Refactor and fix Hint component edge case position issues

Additional info

ethanshar and others added 30 commits December 23, 2024 15:45
@ethanshar
refactor: rename hint rendering methods for clarity
930a844
@ethanshar
fix: add second hint toggle and adjust hint message positioning
084f150
@ethanshar @nitzanyiz
Update src/components/hint/index.tsx
26ef738 
Co-authored-by: Nitzan Yizhar <[email protected]>
@ethanshar
fix: adjust edge margins and target position thresholds in Hint compo…
3414fff 
…nent
@ethanshar
Merge branch 'master' into fix/hint_position
019f317
@ethanshar
Add minimum width to Hint component for better layout consistency
a08f6f5
@ethanshar
Merge branch 'master' into fix/hint_position
f651253
@ethanshar
Convert Hint class component to a function component
5afbf8f
@ethanshar
Separate old Hint code to another file
c2c3d9b
@ethanshar
export error for hint wrap with asBaseComponent
80ed235
@ethanshar
Extract types to another file
40b18be
@ethanshar
Extract animation logic to useHintAnimation
139b8ac
@ethanshar
Add useHintLayout hook to manage hint layout state and measurements
06006e0
@ethanshar
Move HintProps to types
3228021
@ethanshar
cleanups
26ceaac
@ethanshar
Add useHintAccessibility hook for improved accessibility handling
1543cea
@ethanshar
Add useHintPosition hook for dynamic hint positioning logic
cde322d
@ethanshar
Refactor and extract HintMockChildren component
4b38f7b
@ethanshar
Add HintAnchor component for improved hint rendering and management
598312d
@ethanshar
Fix render overlay not rendered
5d230ce
@ethanshar
Refactor Hint components for improved structure and layout handling
56bc7d9
@ethanshar
Refactor Hint components to use LayoutRectangle for target layout and…
4ea0f45 
… improve type definitions
@ethanshar
Refactor Hint components to replace TARGET_POSITIONS with TargetAlign…
a9811ce 
…ments for improved clarity and consistency
@ethanshar
Refactor HintAnchor and useHintPosition components to improve type de…
c6e1eca 
…finitions and enhance clarity in layout handling
@ethanshar
Refactor useHintPosition hook to improve target position calculations…
0b12ec8 
… and enhance clarity in hint positioning logic
@ethanshar
Refactor useHintPosition hook to remove unused targetLayout prop and …
f5016b2 
…optimize target alignment calculations
@ethanshar
Merge branch 'master' into infra/hint_refactor_part3
437ced7
@ethanshar
Refactor useHintLayout hook to accept props directly and improve targ…
e0fb900 
…et layout handling
@ethanshar
Update HintsScreen to rename 'Middle Top' to 'Middle Tip' and improve…
f36d7c7 
… hint key generation for better clarity
@ethanshar
Refactor HintBubble import path and clean up useHintPosition hook by …
ce2d872 
…removing commented-out code and unnecessary padding logic
@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Jan 23, 2025
@Inbal-Tish
Copy link
Collaborator

@ethanshar The hint with right targetPosition and custom content is broken:
Hint-Right-CustomContent-Reactions

@Inbal-Tish Inbal-Tish assigned ethanshar and unassigned Inbal-Tish Jan 29, 2025
@ethanshar
Copy link
Collaborator Author

@ethanshar The hint with right targetPosition and custom content is broken: ![Hint-Right-CustomContent-Reactions]

The issue is in the HintScreen, It didn't update the key when using the custom content feature
Now it does and the calculation works properly

@ethanshar ethanshar assigned Inbal-Tish and unassigned ethanshar Feb 3, 2025
@ethanshar
Copy link
Collaborator Author

@Inbal-Tish @nitzanyiz
Please review this one, we have few GA blockers pending on this

@nitzanyiz
Copy link
Collaborator

@ethanshar , I was wondering if we can keep all the visibility of the hint inside the useHintAnimation hook (maybe rename it to useHintVisibility and that it would return something like {isVisible, visibilityProgress (/AnimatedValue)). We could also keep the

  useDidUpdate(() => {
    animateHint();
  }, [visible]);

inside this hook

@nitzanyiz
Copy link
Collaborator

nitzanyiz commented Feb 9, 2025 •
edited
Loading

Not sure if if we want to handle this in this pr. When rendering hint with a short message in RTL the hint is disconnected from the hint bubble.
The example that is already in the screen. The button, text push button example also.

Hebrew message English message Example that is already in the screen

@nitzanyiz nitzanyiz assigned ethanshar and unassigned nitzanyiz Feb 9, 2025
@ethanshar
Copy link
Collaborator Author

@nitzanyiz
Fixed and added useHintVisibility as suggested

@ethanshar ethanshar assigned nitzanyiz and unassigned ethanshar Feb 10, 2025
Inbal-Tish
Inbal-Tish reviewed Feb 11, 2025
View reviewed changes
demo/src/screens/componentScreens/HintsScreen.tsx
<View marginT-100 row center>
{targetPosition !== 'flex-start' && <Text marginH-s3>Text pushing button</Text>}
<Hint
message={'Hint'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use 'message' so we can text the long massage here as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole idea of this use case was to test short message, that's why it's hard coded
I can change it, I just need the short variation of message to be extra short

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked at it again, I prefer to keep it as it is

Inbal-Tish
Inbal-Tish reviewed Feb 11, 2025
View reviewed changes
demo/src/screens/componentScreens/HintsScreen.tsx
message={'Hint'}
visible={showSecondHint}
onBackgroundPress={this.toggleSecondHint}
useSideTip={false}
Copy link
Collaborator

Choose a reason for hiding this comment

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

useSideTip must be false or we can play with this from the switches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Passing true will result in a wrong UI that looks bad, it's not a bug, it's just not meant for that so I rather pass false

@ethanshar ethanshar merged commit 1381260 into master Feb 12, 2025
1 check passed
@ethanshar ethanshar deleted the infra/hint_refactor_part3 branch February 12, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Inbal-Tish Inbal-Tish Inbal-Tish left review comments

@nitzanyiz nitzanyiz nitzanyiz approved these changes

Assignees

@Inbal-Tish Inbal-Tish

@nitzanyiz nitzanyiz

Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanshar @Inbal-Tish @nitzanyiz