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

fix: editor now detects phone number and launches it #970

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

ARYPROGRAMMER
Copy link
Contributor

Description

This PR addresses Issue #957, ensuring that phone numbers are detected, highlighted, and functional within the AppFlowy Editor. The issue has been resolved, and this PR closes it.


Key Improvements

  • Phone Number Detection: Phone numbers are now automatically detected and highlighted, following the same style as URLs.
  • Interactive Features:
    • On mobile devices, tapping a phone number allows users to:
      • Call the number.
      • Save it to their contacts directory.
  • Non-Intrusive Changes:
    • Existing functionalities for URLs and emails remain unaffected.
    • All code follows established formatting guidelines and includes comments for clarity.

Testing Details

  • Test Results:

    • All existing and new test cases pass successfully.
    • Comprehensive manual testing was conducted to verify functionality across supported platforms.
  • Test Screenshots:
    Test Screenshot 1

    Test Screenshot 2

  • Demonstration:


How to Test

  1. Copy paste a phone number in the editor.
  2. Verify that:
    • The phone number is highlighted in the same way as URLs and emails.
    • Tapping the phone number on a mobile device triggers the appropriate action (call/save).
  3. Confirm that URLs and emails continue to function as expected.

Additional Notes

  • Code Standards: The implementation follows the project's code style guidelines and includes inline comments for better maintainability.
  • Backward Compatibility: No existing functionalities have been disturbed by this update.

Please let me know if additional modifications or improvements are required!

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2024

CLA assistant check
All committers have signed the CLA.

@LucasXu0
Copy link
Collaborator

@ARYPROGRAMMER Can you add a test for the paste_command?

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.23%. Comparing base (5b3878d) to head (2cafc2e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...onent/service/shortcuts/command/paste_command.dart 80.00% 3 Missing ⚠️
...nternal_key_event_handlers/copy_paste_handler.dart 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
- Coverage   72.24%   72.23%   -0.01%     
==========================================
  Files         318      318              
  Lines       15117    15126       +9     
==========================================
+ Hits        10921    10927       +6     
- Misses       4196     4199       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ARYPROGRAMMER
Copy link
Contributor Author

@ARYPROGRAMMER Can you add a test for the paste_command?

Hey, can I add a comprehensive test in another PR after this gets merged, would make it cleaner and more effective instead of adding all in one.

@ARYPROGRAMMER
Copy link
Contributor Author

@ARYPROGRAMMER Can you add a test for the paste_command?

Hey, can I add a comprehensive test in another PR after this gets merged, would make it cleaner and more effective instead of adding all in one.

@ARYPROGRAMMER Can you add a test for the paste_command?

added required tests as tests were specific to the added feat and can be covered easily, new coverage: 69.47%

@ARYPROGRAMMER
Copy link
Contributor Author

@LucasXu0 please review and merge

@LucasXu0
Copy link
Collaborator

I triggered the CI jobs and will merge the PR after all of them pass.

@ARYPROGRAMMER
Copy link
Contributor Author

I triggered the CI jobs and will merge the PR after all of them pass.

let me quickly fix these lints

@ARYPROGRAMMER
Copy link
Contributor Author

@LucasXu0 can you please re-trigger ci, linting checks should pass now

@ARYPROGRAMMER
Copy link
Contributor Author

ARYPROGRAMMER commented Nov 26, 2024

@LucasXu0 , for linting commit, can you discard/change/ignore the unorganized one. also , all important ci still pass

@ARYPROGRAMMER
Copy link
Contributor Author

I triggered the CI jobs and will merge the PR after all of them pass.

just the commit message lint ci shows error since i pushed "added tests..." commit which is not of expected type , rest everything passes

@ARYPROGRAMMER
Copy link
Contributor Author

@LucasXu0 I tried changing commit name via amend, it didn't work, i think this ci would not affect much

@ARYPROGRAMMER
Copy link
Contributor Author

@LucasXu0 any updates on this?

@LucasXu0
Copy link
Collaborator

@ARYPROGRAMMER I can squash the PR. Thanks.

@LucasXu0 LucasXu0 merged commit 566ee6f into AppFlowy-IO:main Nov 27, 2024
6 of 7 checks passed
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.

3 participants